<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 6:55 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 6:45 PM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 6:35 PM Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 10/09/2018 10:03 AM, Jason Ekstrand wrote:<br>
> On Tue, Oct 9, 2018 at 11:24 AM Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a><br>
> <mailto:<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>>> wrote:<br>
> <br>
>     On 10/09/2018 09:00 AM, Jason Ekstrand wrote:<br>
>     > This fixes a bug uncovered by my NIR integer division by constant<br>
>     > optimization series.<br>
>     ><br>
>     > Fixes: 19f9cb72c8b "i965/fs: Add pass to propagate conditional..."<br>
>     > Fixes: 627f94b72e0 "i965/vec4: adding vec4_cmod_propagation..."<br>
>     > Cc: Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a> <mailto:<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>>><br>
>     > ---<br>
>     >  .../compiler/brw_fs_cmod_propagation.cpp      | 25<br>
>     ++++++++++++++++---<br>
>     >  src/intel/compiler/brw_reg.h                  |  9 +++++++<br>
>     >  .../compiler/brw_vec4_cmod_propagation.cpp    | 24 ++++++++++++++++--<br>
>     >  3 files changed, 53 insertions(+), 5 deletions(-)<br>
>     ><br>
>     > diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp<br>
>     b/src/intel/compiler/brw_fs_cmod_propagation.cpp<br>
>     > index 5fb522f810f..4fdd04a9983 100644<br>
>     > --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp<br>
>     > +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp<br>
>     > @@ -25,6 +25,25 @@<br>
>     >  #include "brw_cfg.h"<br>
>     >  #include "brw_eu.h"<br>
>     > <br>
>     > +static bool<br>
>     > +can_add_cmod_to_inst(const fs_inst *inst)<br>
>     > +{<br>
>     > +   if (!inst->can_do_cmod())<br>
>     > +      return false;<br>
>     > +<br>
>     > +   /* The accumulator result appears to get used for the<br>
>     conditional modifier<br>
>     > +    * generation.  When negating a UD value, there is a 33rd bit<br>
>     generated for<br>
>     > +    * the sign in the accumulator value, so now you can't check,<br>
>     for example,<br>
>     > +    * equality with a 32-bit value.  See piglit fs-op-neg-uvec4.<br>
>     > +    */<br>
>     > +   for (unsigned i = 0; i < inst->sources; i++) {<br>
>     > +      if (type_is_unsigned_int(inst->src[i].type) &&<br>
>     inst->src[i].negate)<br>
>     > +         return false;<br>
>     > +   }<br>
>     > +<br>
>     > +   return true;<br>
>     > +}<br>
>     > +<br>
> <br>
>     This probably should go after the @file header comment. :)<br>
> <br>
> <br>
> Yeah, probably.  I'll fix that.<br>
>  <br>
> <br>
>     Also... it looks like this patch replaces every caller of<br>
>     ::can_do_cmod() with can_add_cmod_to_inst.  Maybe just change<br>
>     can_do_cmod?  If you do that, I'd support changing the name to<br>
>     can_add_cmod_to_inst.<br>
> <br>
> <br>
> It does and that was my first attempt.  Unfortunately, we need access to<br>
> the sources for this and we don't have access in backend_instruction so<br>
> this has to be done per-back-end.  I thought about changing can_do_cmod<br>
> to just take an opcode and not an instruction to remove the implication<br>
> that it's somehow comprehensive.<br>
<br>
Oh bother. :(  I'd support that change to can_do_cmod.<br></blockquote><div><br></div><div>Ok, I'll cook something up and send a v2.</div></div></div></blockquote><div><br></div><div>Ugh... I just went and looked at it and there are a half-dozen such helpers that appear to say "can I do a thing" and just switch on the opcode.  Maybe we want them all in brw_eu.h?  I really don't know.  It seems to be something of a mess to me.</div></div></div></blockquote><div><br></div><div>I just had a go at a completely different mechanism which adds can_do_cmod() functions to fs_inst and vec4_instruction like we already do for can_do_source_modifiers.  It still leaves the backend_instruction::can_do_cmod lie there but at leas it's in good company with all the other backend_instruction::can_whatever() helpers that similarly lie?<br></div></div></div>