On Tue, Nov 3, 2015 at 10:14 PM, Kenneth Graunke <kenn...@whitecape.org> wrote: > On Wednesday, October 21, 2015 03:58:15 PM Matt Turner wrote: >> Initially just checks that sources are non-NULL, which would have >> alerted us to the problem fixed by commit 6c846dc5. >> --- >> src/mesa/drivers/dri/i965/Makefile.sources | 1 + >> src/mesa/drivers/dri/i965/brw_eu.h | 4 + >> src/mesa/drivers/dri/i965/brw_eu_validate.c | 150 >> +++++++++++++++++++++++ >> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 8 ++ >> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 8 ++ >> 5 files changed, 171 insertions(+) >> create mode 100644 src/mesa/drivers/dri/i965/brw_eu_validate.c >> >> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources >> b/src/mesa/drivers/dri/i965/Makefile.sources >> index c2438bd..7cd9cc0 100644 >> --- a/src/mesa/drivers/dri/i965/Makefile.sources >> +++ b/src/mesa/drivers/dri/i965/Makefile.sources >> @@ -14,6 +14,7 @@ i965_compiler_FILES = \ >> brw_eu_emit.c \ >> brw_eu.h \ >> brw_eu_util.c \ >> + brw_eu_validate.c \ >> brw_fs_builder.h \ >> brw_fs_channel_expressions.cpp \ >> brw_fs_cmod_propagation.cpp \ >> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h >> b/src/mesa/drivers/dri/i965/brw_eu.h >> index 1345db7..829e393 100644 >> --- a/src/mesa/drivers/dri/i965/brw_eu.h >> +++ b/src/mesa/drivers/dri/i965/brw_eu.h >> @@ -522,6 +522,10 @@ bool brw_try_compact_instruction(const struct >> brw_device_info *devinfo, >> void brw_debug_compact_uncompact(const struct brw_device_info *devinfo, >> brw_inst *orig, brw_inst *uncompacted); >> >> +/* brw_eu_validate.c */ >> +bool brw_validate_instructions(const struct brw_codegen *p, int >> start_offset, >> + struct annotation_info *annotation); >> + >> static inline int >> next_offset(const struct brw_device_info *devinfo, void *store, int offset) >> { >> diff --git a/src/mesa/drivers/dri/i965/brw_eu_validate.c >> b/src/mesa/drivers/dri/i965/brw_eu_validate.c >> new file mode 100644 >> index 0000000..85d4c19 >> --- /dev/null >> +++ b/src/mesa/drivers/dri/i965/brw_eu_validate.c >> @@ -0,0 +1,150 @@ >> +/* >> + * Copyright © 2015 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the next >> + * paragraph) shall be included in all copies or substantial portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >> DEALINGS >> + * IN THE SOFTWARE. >> + */ >> + >> +/** @file brw_eu_validate.c >> + * >> + * This file implements a pass that validates shader assembly. >> + */ >> + >> +#include "brw_eu.h" >> + >> +/* We're going to do lots of string concatenation, so this should help. */ >> +struct string { >> + char *str; >> + size_t len; >> +}; >> + >> +static void >> +cat(struct string *dest, const struct string src) >> +{ >> + dest->str = realloc(dest->str, dest->len + src.len + 1); >> + memcpy(dest->str + dest->len, src.str, src.len); >> + dest->str[dest->len + src.len + 1] = '\0'; >> + dest->len = dest->len + src.len; >> +} >> +#define CAT(dest, src) cat(&dest, (struct string){src, strlen(src)}) >> + >> +#define error(str) "\tERROR: " str "\n" >> + >> +#define ERROR_IF(cond, msg) \ >> + do { \ >> + if (cond) { \ >> + CAT(error_msg, error(msg)); \ >> + valid = false; \ >> + } \ >> + } while(0) > > Are you going to want to support printf-style messages someday? > This infrastructure won't really work for that...error() only handles > string literals...
Hmm. I'm not sure. Seems like it might be useful, but I haven't encountered a case where I want it yet. > > I don't see why you wouldn't just do: > > #define ERROR_F_IF(cond, fmt, ...) \ > do { \ > if (cond) { \ > ralloc_asprintf_rewrite_tail(&error_msg.str, &error_msg.len, \ > "\tERROR: " fmt "\n", __VA_ARGS__); \ > valid = false; \ > } \ > } while(0) > > #define ERROR_IF(cond, msg) ERROR_F_IF(cond, "%s", msg) > > Then ralloc_free(error_msg.str) later instead of free(). > > It's more flexible and avoids the need for cat(), CAT(), and error(). That might be good. >> + >> +static bool >> +src0_is_null(const struct brw_device_info *devinfo, const brw_inst *inst) >> +{ >> + return brw_inst_src0_reg_file(devinfo, inst) == >> BRW_ARCHITECTURE_REGISTER_FILE && >> + brw_inst_src0_da_reg_nr(devinfo, inst) == BRW_ARF_NULL; >> +} >> + >> +static bool >> +src1_is_null(const struct brw_device_info *devinfo, const brw_inst *inst) >> +{ >> + return brw_inst_src1_reg_file(devinfo, inst) == >> BRW_ARCHITECTURE_REGISTER_FILE && >> + brw_inst_src1_da_reg_nr(devinfo, inst) == BRW_ARF_NULL; >> +} >> + >> +static unsigned >> +num_sources_from_inst(const struct brw_device_info *devinfo, >> + const brw_inst *inst) >> +{ >> + unsigned math_function; >> + >> + if (brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MATH) { >> + math_function = brw_inst_math_function(devinfo, inst); >> + } else if (devinfo->gen < 6 && >> + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_SEND) { >> + if (brw_inst_sfid(devinfo, inst) == BRW_SFID_MATH) { >> + math_function = brw_inst_math_msg_function(devinfo, inst); >> + } else { >> + return 0; > > Technically, Gen4-5 SEND instructions can have 1 source, which is > implicitly MOV'd to inst->base_mrf...but I guess you don't want to > handle that here, as the source is optional (so ARF NULL is OK). Yeah, exactly. > I hope you like my suggestions, but either way... > > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> Thanks! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev