[Mesa-dev] [PATCH] intel/fs: Bail in optimize_extract_to_float if we have modifiers

[Jason Ekstrand]

On Thu, Feb 14, 2019 at 1:18 PM Matt Turner <mattst88 at gmail.com> wrote:

> On Mon, Feb 11, 2019 at 8:44 PM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> >
> > This fixes a bug in runscape where we were optimizing x >> 16 to an
> > extract and then negating and converting to float.  The NIR to fs pass
> > was dropping the negate on the floor breaking a geometry shader and
> > causing it to render nothing.
> >
> > Fixes: 1f862e923cb "i965/fs: Optimize float conversions of byte/word..."
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109601
> > Cc: Matt Turner <mattst88 at gmail.com>
> > ---
> >  src/intel/compiler/brw_fs_nir.cpp | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> > index b80f4351b49..204640ac726 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -510,6 +510,11 @@ fs_visitor::optimize_extract_to_float(nir_alu_instr
> *instr,
> >         src0->op != nir_op_extract_i8 && src0->op != nir_op_extract_i16)
> >        return false;
> >
> > +   /* If either opcode has source modifiers, bail. */
> > +   if (instr->src[0].abs || instr->src[0].negate ||
> > +        src0->src[0].abs || src0->src[0].negate)
>
> You've done something weird here to vertically align things. I don't
> know if I like it, but if you're doing to indent the 'src0' so that
> the rest aligns then at least be consistent and align both 'src0's :)
>

That wasn't intentional.  I've changed it back to the conventional
indentation so nothing lines up.


> > +      return false;
> > +
>
> We could handle this better by allowing the optimization if we're
> extracting the high byte/word. In that case I think we could safely
> apply the source mod. We should do that separately, but it might be
> nice to leave a FINISHME here so we'll remember.
>

I dropped the following comment in:

+   /* If either opcode has source modifiers, bail.
+    *
+    * TODO: We can potentially handle source modifiers if both of the
opcodes
+    * we're combining are signed integers.
+    */


> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>

Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190214/5fbefe68/attachment.html>