[Mesa-dev] [PATCH 2/2] i965: Fix typo in nir_op_pack_double_2x32_split handling

[Iago Toral]

On Fri, 2016-10-14 at 10:23 -0700, Ian Romanick wrote:
> On 10/08/2016 09:33 AM, Eduardo Lima Mitev wrote:
> > 
> > On 10/08/2016 02:12 AM, Ian Romanick wrote:
> > > 
> > > From: Ian Romanick <ian.d.romanick at intel.com>
> > > 
> > > This was found partially by inspection and partially by hitting a
> > > problem while working on nir_op_pack_int64_2x32_split.  The code
> > > previously would 'continue' if (instr->src[i].src.is_ssa), but
> > > the code
> > > immediately following in the loop treats instr->src[i] as an SSA
> > > value.
> > > 
> > > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > > Cc: mesa-stable at lists.freedesktop.org
> > > Cc: Iago Toral Quiroga <itoral at igalia.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > > index 4e68ffb..2cbcab1 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > > @@ -1208,7 +1208,7 @@ fs_visitor::nir_emit_alu(const fs_builder
> > > &bld, nir_alu_instr *instr)
> > >         * the unpack operation.
> > >         */
> > >        for (int i = 0; i < 2; i++) {
> > > -         if (instr->src[i].src.is_ssa)
> > > +         if (!instr->src[i].src.is_ssa)
> > >              continue;
> > >  
> > Good catch!
> But maybe not.  Re-running this through the CI shows about 1,000 test
> regressions due to assertion failures.  I looked at the rest of the
> loop, and I'm really not sure how this works:

Urgh, what a mess, I think it is very likely that I initially wrote
this optimization like this:

if (is_ssa) {
   ...
   if (is_alu) {
      ...
   }
}

And eventually decided to change that to this style:

if (!is_ssa)
  continue:

if (!is_alu)
  continue:

...

But forgot to change all the conditions, resulting in the opt never
running again. We did not notice this because thanks to Curro's work on
fixing spilling on the FS backend with his SIMD32 patches, the piglit
tests that would fail to compile without this optimization started to
spill just fine and pass without this.

I am sending a patch to get this fixed. Even if the nir algebraic opt
pass seems like a better place to do this I guess we want this fixed
until we do that.

This is what is wrong:

>       for (int i = 0; i < 2; i++) {
>          if (instr->src[i].src.is_ssa)
>             continue;

This should be:

if (!instr->src[i].src.is_ssa)
   continue;

>          const nir_instr *parent_instr = instr->src[i].src.ssa-
> >parent_instr;
> 
> We skip this if the source is SSA, but then we use it as SSA.
> 
>          if (parent_instr->type == nir_instr_type_alu)
>             continue;

This should be:

if (parent_instr->type != nir_instr_type_alu)
   continue;

>          const nir_alu_instr *alu_parent =
> nir_instr_as_alu(parent_instr);
> 
> We skip this if the parent is ALU, but then we use it as ALU.  The
> assertion failure occurs inside nir_instr_as_alu.
> 
>          if (alu_parent->op == nir_op_unpack_double_2x32_split_x ||
>              alu_parent->op == nir_op_unpack_double_2x32_split_y)
>             continue;

This should be != && !=

>          if (!alu_parent->src[0].src.is_ssa)
>             continue;

This one is correct.

>          op[i] = get_nir_src(alu_parent->src[0].src);
>          op[i] = offset(retype(op[i], BRW_REGISTER_TYPE_DF), bld,
>                         alu_parent->src[0].swizzle[channel]);
>          if (alu_parent->op == nir_op_unpack_double_2x32_split_y)
>             op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 1);
>          else
>             op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 0);
>       }
> 
> Were you guys ever able to make this optimization trigger?  I suspect
> that the very first continue always occurs, so none of this actually
> happens.  Either way, it seems like this optimization should happen
> in
> nir_opt_algebraic instead.

I was able to make it trigger when I wrote the initial version. In fact
I wrote this because otherwise, before the spilling fixes that Curro
implemented in the FS backend, without this optimization some tests (I
think some varying-packing tests for doubles) would fail to compile. I
remember this went through a few changes during review and I probably
messed it up at some point.

> This has come up because I need to do something similar for
> int64.  All
> of the lowering passes for int64 will generate a lot of
> unpack(pack(...)) type sequences.  I'm doing the lowering in GLSL IR,
> so I've also done the algebraic optimization in GLSL IR.
> 
> > 
> > Both patches are:
> > 
> > Reviewed-by: Eduardo Lima Mitev <elima at igalia.com>
> > 
> > > 
> > >           const nir_instr *parent_instr = instr->src[i].src.ssa-
> > > >parent_instr;
> > > 
>