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

[Jason Ekstrand]

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.

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