<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 11:24 AM Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 10/09/2018 09:00 AM, Jason Ekstrand wrote: > This fixes a bug uncovered by my NIR integer division by constant > optimization series. > > Fixes: 19f9cb72c8b "i965/fs: Add pass to propagate conditional..." > Fixes: 627f94b72e0 "i965/vec4: adding vec4_cmod_propagation..." > Cc: Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>> > --- > .../compiler/brw_fs_cmod_propagation.cpp | 25 ++++++++++++++++--- > src/intel/compiler/brw_reg.h | 9 +++++++ > .../compiler/brw_vec4_cmod_propagation.cpp | 24 ++++++++++++++++-- > 3 files changed, 53 insertions(+), 5 deletions(-) > > diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp > index 5fb522f810f..4fdd04a9983 100644 > --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp > +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp > @@ -25,6 +25,25 @@ > #include "brw_cfg.h" > #include "brw_eu.h" > > +static bool > +can_add_cmod_to_inst(const fs_inst *inst) > +{ > + if (!inst->can_do_cmod()) > + return false; > + > + /* The accumulator result appears to get used for the conditional modifier > + * generation. When negating a UD value, there is a 33rd bit generated for > + * the sign in the accumulator value, so now you can't check, for example, > + * equality with a 32-bit value. See piglit fs-op-neg-uvec4. > + */ > + for (unsigned i = 0; i < inst->sources; i++) { > + if (type_is_unsigned_int(inst->src[i].type) && inst->src[i].negate) > + return false; > + } > + > + return true; > +} > + This probably should go after the @file header comment. :) </blockquote><div> </div><div>Yeah, probably. I'll fix that. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Also... it looks like this patch replaces every caller of ::can_do_cmod() with can_add_cmod_to_inst. Maybe just change can_do_cmod? If you do that, I'd support changing the name to can_add_cmod_to_inst. </blockquote><div> </div><div>It does and that was my first attempt. Unfortunately, we need access to the sources for this and we don't have access in backend_instruction so this has to be done per-back-end. I thought about changing can_do_cmod to just take an opcode and not an instruction to remove the implication that it's somehow comprehensive. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > /** @file brw_fs_cmod_propagation.cpp > * > * Implements a pass that propagates the conditional modifier from a CMP x 0.0 > @@ -91,7 +110,7 @@ cmod_propagate_cmp_to_add(const gen_device_info *devinfo, bblock_t *block, > negate ? brw_swap_cmod(inst->conditional_mod) > : inst->conditional_mod; > > - if (scan_inst->can_do_cmod() && > + if (can_add_cmod_to_inst(scan_inst) && > ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) || > scan_inst->conditional_mod == cond)) { > scan_inst->conditional_mod = cond; > @@ -146,7 +165,7 @@ cmod_propagate_not(const gen_device_info *devinfo, bblock_t *block, > scan_inst->exec_size != inst->exec_size) > break; > > - if (scan_inst->can_do_cmod() && > + if (can_add_cmod_to_inst(scan_inst) && > ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) || > scan_inst->conditional_mod == cond)) { > scan_inst->conditional_mod = cond; > @@ -316,7 +335,7 @@ opt_cmod_propagation_local(const gen_device_info *devinfo, bblock_t *block) > inst->src[0].negate ? brw_swap_cmod(inst->conditional_mod) > : inst->conditional_mod; > > - if (scan_inst->can_do_cmod() && > + if (can_add_cmod_to_inst(scan_inst) && > ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) || > scan_inst->conditional_mod == cond)) { > scan_inst->conditional_mod = cond; > diff --git a/src/intel/compiler/brw_reg.h b/src/intel/compiler/brw_reg.h > index ac12ab3d2dd..46d66198a1d 100644 > --- a/src/intel/compiler/brw_reg.h > +++ b/src/intel/compiler/brw_reg.h > @@ -376,6 +376,15 @@ brw_int_type(unsigned sz, bool is_signed) > } > } > > +static inline bool > +type_is_unsigned_int(enum brw_reg_type tp) > +{ > + return tp == BRW_REGISTER_TYPE_UB || > + tp == BRW_REGISTER_TYPE_UW || > + tp == BRW_REGISTER_TYPE_UD || > + tp == BRW_REGISTER_TYPE_UQ; > +} > + > /** > * Construct a brw_reg. > * \param file one of the BRW_x_REGISTER_FILE values > diff --git a/src/intel/compiler/brw_vec4_cmod_propagation.cpp b/src/intel/compiler/brw_vec4_cmod_propagation.cpp > index a1d46dc8dca..392e283d1f8 100644 > --- a/src/intel/compiler/brw_vec4_cmod_propagation.cpp > +++ b/src/intel/compiler/brw_vec4_cmod_propagation.cpp > @@ -46,6 +46,26 @@ writemasks_incompatible(const vec4_instruction *earlier, > (later->dst.writemask & ~earlier->dst.writemask) != 0; > } > > +static bool > +can_add_cmod_to_inst(const vec4_instruction *inst) > +{ > + if (!inst->can_do_cmod()) > + return false; > + > + /* The accumulator result appears to get used for the conditional modifier > + * generation. When negating a UD value, there is a 33rd bit generated for > + * the sign in the accumulator value, so now you can't check, for example, > + * equality with a 32-bit value. See piglit fs-op-neg-uvec4. > + */ > + for (unsigned i = 0; i < 3; i++) { > + if (inst->src[i].file != BAD_FILE && > + type_is_unsigned_int(inst->src[i].type) && inst->src[i].negate) > + return false; > + } > + > + return true; > +} > + > static bool > opt_cmod_propagation_local(bblock_t *block) > { > @@ -132,7 +152,7 @@ opt_cmod_propagation_local(bblock_t *block) > negate ? brw_swap_cmod(inst->conditional_mod) > : inst->conditional_mod; > > - if (scan_inst->can_do_cmod() && > + if (can_add_cmod_to_inst(scan_inst) && > ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) || > scan_inst->conditional_mod == cond)) { > scan_inst->conditional_mod = cond; > @@ -229,7 +249,7 @@ opt_cmod_propagation_local(bblock_t *block) > inst->src[0].negate ? brw_swap_cmod(inst->conditional_mod) > : inst->conditional_mod; > > - if (scan_inst->can_do_cmod() && > + if (can_add_cmod_to_inst(scan_inst) && > ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) || > scan_inst->conditional_mod == cond)) { > scan_inst->conditional_mod = cond; > </blockquote></div></div>