On Wed, May 23, 2018 at 3:40 PM, Francisco Jerez <[email protected]> wrote:
> Jason Ekstrand <[email protected]> writes: > > > On Wed, May 23, 2018 at 12:13 PM, Francisco Jerez <[email protected] > > > > wrote: > > > >> Jason Ekstrand <[email protected]> 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: [email protected] > >> > --- > >> > 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 > >> > [email protected] > >> > https://lists.freedesktop.org/mailman/listinfo/mesa-stable > >> >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
