Re: [PATCH 3/3] Support APX zero-upper

[Jan Beulich <jbeulich@suse.com>]

On 20.05.2024 08:22, Cui, Lili wrote:
> gas/ChangeLog:

I did specifically ask for (at least) one thing to be added to the description
here. You put that in the cover letter, which means it'll not end up in the
eventual commit. Please put it here. And as before, please try to get used to
not, ever, submit patches without any description, unless the title alone
really describes it all (including e.g. implementation decisions taken). What
you ...

>         * config/tc-i386.c (build_apx_evex_prefix): Handle ZU.
>         * testsuite/gas/i386/x86-64.exp: Added new tests for ZU.
>         * testsuite/gas/i386/x86-64.exp: Added new tests for ZU.
>         * testsuite/gas/i386/x86-64-apx-zu-intel.d: New test.
>         * testsuite/gas/i386/x86-64-apx-zu-inval.l: Ditto.
>         * testsuite/gas/i386/x86-64-apx-zu-inval.s: Ditto.
>         * testsuite/gas/i386/x86-64-apx-zu.d: Ditto.
>         * testsuite/gas/i386/x86-64-apx-zu.s: Ditto.
> 
> opcodes/ChangeLog:
> 
>         * i386-dis-evex-prefix.h: Handle PREFIX_EVEX_MAP4_40 ~
>         PREFIX_EVEX_MAP4_4F.
>         * i386-dis-evex.h: Ditto.
>         * i386-dis.c (struct dis386): Add new micro 'ZU'.
>         (putop): Handle %ZU.
>         * i386-gen.c: Added ZU.
>         * i386-opc.h: Ditto.
>         * i386-opc.tbl: Added new templates to support ZU.

... mechanically put here is useful to see _what_ was changed, but it's
entirely unhelpful when one wants to understand _why_ things were done
(perhaps a certain way).

> @@ -10877,6 +10894,18 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>  	      abort ();
>  	    }
>  	  break;
> +	case 'U':
> +	  if (l == 1 && (last[0] == 'Z'))
> +	    {
> +	      /* Although IMUL/SETcc does not support NDD, the EVEX.ND bit is
> +		 used to control whether its destination register has its upper
> +		 bits zeroed when OSIZE is 16b/8b.  */
> +	      if (ins->vex.nd)
> +		oappend (ins, "zu");

I find in particular the OSIZE part of the comment somewhat misleading. For
SETcc operand size (irrespective of any further insn attributes like
[embedded] prefixes) is 8 bits. Nothing to say there at all in this regard.
And for IMUL while EVEX.ZU indeed has an effect for 16-bit operand size
only, what about the bit being set when operand size is 32 or 64 bits? If
the doc is to be trusted, the this is benign, and afaict you follow that by
emitting "zu" in such cases as well. Just that the comment suggests
otherwise.

Once again: Okay with respective adjustments.

Jan