[Mesa-dev] [Mesa-stable] [PATCH] intel/fs: Properly copy default flag reg for 3src instrucitons

[Jason Ekstrand]

On Wed, May 23, 2018 at 12:13 PM, Francisco Jerez <currojerez at riseup.net>
wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > Prior to gen8, the flag reg and subreg numbers are in different
> > locations on 3src instructions than on smaller instructions.  In order
> > for brw_set_default_flag_reg to work properly, we need to copy the value
> > out of the 2src location and write it into the 3src location as part of
> > brw_alu3.
> >
> > Cc: mesa-stable at lists.freedesktop.org
> > ---
> >  src/intel/compiler/brw_eu_emit.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/intel/compiler/brw_eu_emit.c
> b/src/intel/compiler/brw_eu_emit.c
> > index 4f51d51..fc39d94 100644
> > --- a/src/intel/compiler/brw_eu_emit.c
> > +++ b/src/intel/compiler/brw_eu_emit.c
> > @@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode,
> struct brw_reg dest,
> >               dest.type == BRW_REGISTER_TYPE_DF ||
> >               dest.type == BRW_REGISTER_TYPE_D  ||
> >               dest.type == BRW_REGISTER_TYPE_UD);
> > +
> > +      /* Flag registers are in a different spot on 3src instructions so
> we
> > +       * need to move the value if we want brw_set_default_flag_reg to
> work
> > +       * properly.
> > +       */
> > +      unsigned flag_reg_nr =
> > +         devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;
> > +      unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(devinfo, inst);
> > +      if (devinfo->gen >= 7)
> > +         brw_inst_set_3src_a16_flag_reg_nr(devinfo, inst, flag_reg_nr);
> > +      brw_inst_set_3src_a16_flag_subreg_nr(devinfo, inst,
> flag_subreg_nr);
> > +
>
> This seems really gross...


Yes, yes it is.  The grossness is not lost on me.


> brw_next_insn() with a 3-source opcode gives
> you a corrupted 3-source instruction initialized as if it were a
> 2-source one.  This fixes the 3-source flag register field hoping that
> all other fields of p->current will magically match the 3-source
> instruction layout, and hoping that the stray bits of the 2-source flag
> register field will be overwritten by something else in this function.
>

Yes, it does.  However, I did go through and check and I believe that the
flag [sub]reg number field is the only field that actually moves.  Of
course, this is not something we can guarantee going forward, but I think
we're ok today.


> Proper fix would be to get rid of the p->current thing altogether IMO
> and use a mechanism to track instruction defaults based on their
> semantics rather than on the binary encoding of an instruction which has
> unknown encoding...


Agreed.  It really bothered me when I found this bug and I think we would
benefit significantly from having a semantic stack rather than this
"default instruction" concept.


> That would be rather invasive though, a compromise
> might be to fix it in brw_next_insn() by starting with a clean
> instruction and initializing it from scratch with the 3src helpers...
>

Yes, fixing it in brw_next_insn() might be a better place.  Given that all
the other fields match up, I'm a bit inclined to just move this fix to
brw_next_insn() for now and plan to follow up with a logical instruction
control stack.  Would that be ok?


> >        if (devinfo->gen == 6) {
> >           brw_inst_set_3src_a16_dst_reg_file(devinfo, inst,
> >                                              dest.file ==
> BRW_MESSAGE_REGISTER_FILE);
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-stable mailing list
> > mesa-stable at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-stable
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180523/db733957/attachment.html>