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

[Francisco Jerez]

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.

>
>> >        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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180523/d7ac3ea5/attachment.sig>