<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 23, 2018 at 12:13 PM, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> Prior to gen8, the flag reg and subreg numbers are in different<br>
> locations on 3src instructions than on smaller instructions. In order<br>
> for brw_set_default_flag_reg to work properly, we need to copy the value<br>
> out of the 2src location and write it into the 3src location as part of<br>
> brw_alu3.<br>
><br>
> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
> ---<br>
> src/intel/compiler/brw_eu_<wbr>emit.c | 12 ++++++++++++<br>
> 1 file changed, 12 insertions(+)<br>
><br>
> diff --git a/src/intel/compiler/brw_eu_<wbr>emit.c b/src/intel/compiler/brw_eu_<wbr>emit.c<br>
> index 4f51d51..fc39d94 100644<br>
> --- a/src/intel/compiler/brw_eu_<wbr>emit.c<br>
> +++ b/src/intel/compiler/brw_eu_<wbr>emit.c<br>
> @@ -824,6 +824,18 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct brw_reg dest,<br>
> dest.type == BRW_REGISTER_TYPE_DF ||<br>
> dest.type == BRW_REGISTER_TYPE_D ||<br>
> dest.type == BRW_REGISTER_TYPE_UD);<br>
> +<br>
> + /* Flag registers are in a different spot on 3src instructions so we<br>
> + * need to move the value if we want brw_set_default_flag_reg to work<br>
> + * properly.<br>
> + */<br>
> + unsigned flag_reg_nr =<br>
> + devinfo->gen >= 7 ? brw_inst_flag_reg_nr(devinfo, inst) : 0;<br>
> + unsigned flag_subreg_nr = brw_inst_flag_subreg_nr(<wbr>devinfo, inst);<br>
> + if (devinfo->gen >= 7)<br>
> + brw_inst_set_3src_a16_flag_<wbr>reg_nr(devinfo, inst, flag_reg_nr);<br>
> + brw_inst_set_3src_a16_flag_<wbr>subreg_nr(devinfo, inst, flag_subreg_nr);<br>
> +<br>
<br>
</div></div>This seems really gross...</blockquote><div><br></div><div>Yes, yes it is. The grossness is not lost on me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">brw_next_insn() with a 3-source opcode gives<br>
you a corrupted 3-source instruction initialized as if it were a<br>
2-source one. This fixes the 3-source flag register field hoping that<br>
all other fields of p->current will magically match the 3-source<br>
instruction layout, and hoping that the stray bits of the 2-source flag<br>
register field will be overwritten by something else in this function.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Proper fix would be to get rid of the p->current thing altogether IMO<br>
and use a mechanism to track instruction defaults based on their<br>
semantics rather than on the binary encoding of an instruction which has<br>
unknown encoding...</blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">That would be rather invasive though, a compromise<br>
might be to fix it in brw_next_insn() by starting with a clean<br>
instruction and initializing it from scratch with the 3src helpers...<span class=""><br></span></blockquote><div><br></div><div>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?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> if (devinfo->gen == 6) {<br>
> brw_inst_set_3src_a16_dst_reg_<wbr>file(devinfo, inst,<br>
> dest.file == BRW_MESSAGE_REGISTER_FILE);<br>
> -- <br>
> 2.5.0.400.gff86faf<br>
><br>
</span>> ______________________________<wbr>_________________<br>
> mesa-stable mailing list<br>
> <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-stable" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-stable</a><br>
</blockquote></div><br></div></div>