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

[Francisco Jerez]

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...  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.

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...  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...

>        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/145905aa/attachment.sig>