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

[Jason Ekstrand]

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

> 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.
>

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181009/86053af8/attachment-0001.html>