On Wed, May 23, 2018 at 12:13 PM, Francisco Jerez <curroje...@riseup.net> wrote:
> Jason Ekstrand <ja...@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-sta...@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-sta...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-stable >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev