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

[Jason Ekstrand]

On Fri, Oct 14, 2016 at 10:23 AM, Ian Romanick <idr at freedesktop.org> 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:
>
>       for (int i = 0; i < 2; i++) {
>          if (instr->src[i].src.is_ssa)
>             continue;
>

Yeah, this should be !


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

and this should be !=

if it works at all...


>             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;
>
>          if (!alu_parent->src[0].src.is_ssa)
>             continue;
>
>          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've never been a huge fan of this optimization.  My brain's too dead on a
Friday to think about all of the implications, but it's obviously not
working as-is.


> 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;
> >>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161014/1ecb26da/attachment.html>