[Mesa-dev] [PATCH] intel: Don't propagate conditional modifiers if a UD source is negated

[Jason Ekstrand]

On Tue, Oct 9, 2018 at 6:45 PM Jason Ekstrand <jason at jlekstrand.net> wrote:

> On Tue, Oct 9, 2018 at 6:35 PM Ian Romanick <idr at freedesktop.org> wrote:
>
>> On 10/09/2018 10:03 AM, Jason Ekstrand wrote:
>> > On Tue, Oct 9, 2018 at 11:24 AM Ian Romanick <idr at freedesktop.org
>> > <mailto:idr at freedesktop.org>> wrote:
>> >
>> >     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 <mattst88 at gmail.com <mailto:mattst88 at gmail.com>>
>> >     > ---
>> >     >  .../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. :)
>> >
>> >
>> > Yeah, probably.  I'll fix that.
>> >
>> >
>> >     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.
>> >
>> >
>> > 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.
>>
>> Oh bother. :(  I'd support that change to can_do_cmod.
>>
>
> Ok, I'll cook something up and send a v2.
>

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.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181009/5e1cb8c0/attachment.html>