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

[Jason Ekstrand]

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

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > 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?
> >
>
> Yeah, I suppose, but this patch still only fixes half of the problem,
> there are still bogus bits set on the 3-source instruction by the memcpy
> from p->current, which we rely on other code overwriting for
> correctness, which seems extremely fragile (assuming it's working at all
> for all codepaths below).  It would be better (and probably
> straightforward enough for it to make it to stable releases) to
> initialize the new instruction to zero in brw_next_insn() and initialize
> the instruction fields manually by using the 3src helpers in case we get
> a 3-source opcode.
>

Yeah.  I hadn't thought about other bits carying over. :(  I'll see what we
I can do about just getting rid of the memcpy for 3src instructions.


> >
> >> >        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/eb3686a3/attachment-0001.html>