RE: [PATCH] Support {evex} pseudo prefix for decode evex promoted insns without egpr32.

["Cui, Lili" <lili.cui@intel.com>]

> On 06.03.2024 10:58, Cui, Lili wrote:
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/noreg64-evex.s
> 
> This separate test, not directly fitting with the patch title, wants mentioning in
> the patch description.
> 

Ok.

> > @@ -0,0 +1,67 @@
> > +# Check 64-bit insns not sizeable through register operands with evex
> > +	.text
> > +	{evex} adc	$1, (%rax)
> > +	{evex} adc	$0x89, (%rax)
> > +	{evex} adc	$0x1234, (%rax)
> > +	{evex} adc	$0x12345678, (%rax)
> > +	{evex} add	$1, (%rax)
> > +	{evex} add	$0x89, (%rax)
> > +	{evex} add	$0x1234, (%rax)
> > +	{evex} add	$0x12345678, (%rax)
> > +	{evex} and	$1, (%rax)
> > +	{evex} and	$0x89, (%rax)
> > +	{evex} and	$0x1234, (%rax)
> > +	{evex} and	$0x12345678, (%rax)
> > +	{evex} crc32	(%rax), %eax
> 
> noreg64.s tests %rax as a destination separately, for a reason.

I don't know the reason why noreg64.s uses %rax, so it just follows the previous style. Do you know why? Is it not necessary to keep using %rax in the current test file?

> > +	{evex} sbb	$1, (%rax)
> > +	{evex} sbb	$0x89, (%rax)
> > +	{evex} sbb	$0x1234, (%rax)
> > +	{evex} sbb	$0x12345678, (%rax)
> > +	{evex} sar	$1, (%rax)
> 
> Like noreg64.s please have "sal" tests here, too.
> 

Added.

> > --- a/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.d
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.d
> > @@ -118,7 +118,7 @@ Disassembly of section .text:
> >  \s*[a-f0-9]+:\s*67 0f 4d 90 90 90 90 90 	cmovge -
> 0x6f6f6f70\(%eax\),%edx
> >  \s*[a-f0-9]+:\s*67 0f 4e 90 90 90 90 90 	cmovle -
> 0x6f6f6f70\(%eax\),%edx
> >  \s*[a-f0-9]+:\s*67 0f 4f 90 90 90 90 90 	cmovg  -
> 0x6f6f6f70\(%eax\),%edx
> > -\s*[a-f0-9]+:\s*62 f4 7d 08 60 c0    	movbe  %ax,%ax
> > +\s*[a-f0-9]+:\s*62 f4 7d 08 60 c0    	\{evex\} movbe %ax,%ax
> 
> This is wrong: An {evex} prefix should appear on MOVBE only when there's a
> memory operand (and no use of an eGPR).

OK, I will fix it.

> 
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx_f-evex-intel.d
> > @@ -0,0 +1,1197 @@
> > +#as:
> > +#objdump: -dw -Mintel
> > +#name: x86_64 APX_F insns with evex pseudo prefix (Intel disassembly)
> 
> Is an Intel disassembly variant of this test really worth it? It's not exactly small,
> after all. If you want to keep it, ...
> 

I prefer to drop it.

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx_f-evex.d
> > @@ -0,0 +1,1197 @@
> > +#as:
> > +#objdump: -dw
> > +#name: x86_64 APX_F insns with evex pseudo prefix
> > +#source: x86-64-apx_f-evex.s
> > +
> > +.*: +file format .*
> > +
> > +Disassembly of section \.text:
> > +
> > +0+ <_start>:
> > +\s*[a-f0-9]+:\s*62 54 fc 08 fc bc 80 23 01 00 00\s+\{evex\}
> > +aadd\s+%r15,0x123\(%r8,%rax,4\)
> 
> ... please bring its expectations in line with this (just a single blank between
> {evex} and the insn menmonic).
> 

Changed x86-64-apx_f-evex.s  to use space instead of tab between {evex} and mnemonic.

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx_f-evex.s
> > @@ -0,0 +1,1192 @@
> > +# Check 64bit APX_F instructions with evex pseudo prefix
> > +
> > +	{evex}	imul	%r15
> > +	{evex}	imul	%r15,%r15
> > +	{evex}	imul	%r15d
> > +	{evex}	imul	%r15d,%edx
> > +	{evex}	imul	%r15w
> > +	{evex}	imul	%r15w,%ax
> > +	{evex}	imul	%r8b
> > +	{evex}	imulb	0x123(%r8,%rax,4)
> > +	{evex}	imulw	0x123(%r8,%rax,4)
> > +	{evex}	imull	0x123(%r8,%rax,4)
> > +	{evex}	imul	0x123(%r8,%rax,4),%r15
> > +	{evex}	imul	0x123(%r8,%rax,4),%r15d
> > +	{evex}	imul	0x123(%r8,%rax,4),%r15w
> > +	{evex}	imulq	0x123(%r8,%rax,4)
> > +	{evex}	imul	$0x7b, %dx, %ax
> > +	{evex}	imul	$0x7b, %ecx, %edx
> > +	{evex}	imul	$0x7b, %r9, %r15
> > +	{evex}	imul	$0x7b, 291(%r8, %rax, 4), %dx
> > +	{evex}	imul	$0x7b, 291(%r8, %rax, 4), %ecx
> > +	{evex}	imul	$0x7b, 291(%r8, %rax, 4), %r9
> > +	{evex}	imul	$0xff90, %dx, %ax
> > +	{evex}	imul	$0xff90, %ecx, %edx
> > +	{evex}	imul	$0xff90, %r9, %r15
> > +	{evex}	imul	$0xff90, 291(%r8, %rax, 4), %dx
> > +	{evex}	imul	$0xff90, 291(%r8, %rax, 4), %ecx
> > +	{evex}	imul	$0xff90, 291(%r8, %rax, 4), %r9
> 
> Also covering the 2-operand forms of these (srcreg == dstreg) would seem
> desirable.

I'm a little confused here, do you suggest adding such a testcase?

{evex}	imul	$0x7b, %r9, %r9

> 
> > +	{evex}	movbe	%r15,%r15
> 
> No {evex} needed here, ...
> 
> > +	{evex}	movbe	%r15,0x123(%r8,%rax,4)
> > +	{evex}	movbe	%r15d,%edx
> 
> ... here, or ...
> 
> > +	{evex}	movbe	%r15d,0x123(%r8,%rax,4)
> > +	{evex}	movbe	%r15w,%ax
> 
> ... here (iow these three can be omitted).

Ok, I'll remove them from this test file and add them to x86-64-apx-evex-promoted.s.

> > +	{evex}	ror	$0x7b,%r15
> > +	{evex}	ror	$0x7b,%r15d
> > +	{evex}	ror	$0x7b,%r15w
> > +	{evex}	ror	$0x7b,%r8b
> > +	{evex}	rorb	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	rorw	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	rorl	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	rorq	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	ror	$1,%r15
> > +	{evex}	ror	$1,%r15d
> > +	{evex}	ror	$1,%r15w
> > +	{evex}	ror	$1,%r8b
> > +	{evex}	rorb	$1,0x123(%r8,%rax,4)
> > +	{evex}	rorw	$1,0x123(%r8,%rax,4)
> > +	{evex}	rorl	$1,0x123(%r8,%rax,4)
> > +	{evex}	rorq	$1,0x123(%r8,%rax,4)
> > +	{evex}	ror	%cl,%r15
> > +	{evex}	ror	%cl,%r15d
> > +	{evex}	ror	%cl,%r15w
> > +	{evex}	ror	%cl,%r8b
> > +	{evex}	rorb	%cl,0x123(%r8,%rax,4)
> > +	{evex}	rorw	%cl,0x123(%r8,%rax,4)
> > +	{evex}	rorl	%cl,0x123(%r8,%rax,4)
> > +	{evex}	rorq	%cl,0x123(%r8,%rax,4)
> > +	{evex}	rorx	$0x7b,%r15,%r15
> > +	{evex}	rorx	$0x7b,%r15d,%edx
> > +	{evex}	rorx	$0x7b,0x123(%r8,%rax,4),%r15
> > +	{evex}	rorx	$0x7b,0x123(%r8,%rax,4),%r15d
> > +	{evex}	sar	$0x7b,%r15
> 
> Please also again cover "sal".
> 

Ok.

> > +	{evex}	xor	$0x7b,%r15
> > +	{evex}	xor	$0x7b,%r15d
> > +	{evex}	xor	$0x7b,%r15w
> > +	{evex}	xor	$0x7b,%r8b
> > +	{evex}	xorb	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	xorw	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	xorl	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	xorq	$0x7b,0x123(%r8,%rax,4)
> > +	{evex}	xor	%r15,%r15
> > +	{evex}	xor	%r15,0x123(%r8,%rax,4)
> > +	{evex}	xor	%r15d,%edx
> > +	{evex}	xor	%r15d,0x123(%r8,%rax,4)
> > +	{evex}	xor	%r15w,%ax
> > +	{evex}	xor	%r15w,0x123(%r8,%rax,4)
> > +	{evex}	xor	%r8b,%dl
> > +	{evex}	xor	%r8b,0x123(%r8,%rax,4)
> > +	{evex}	xor	0x123(%r8,%rax,4),%r15
> > +	{evex}	xor	0x123(%r8,%rax,4),%r15d
> > +	{evex}	xor	0x123(%r8,%rax,4),%r15w
> > +	{evex}	xor	0x123(%r8,%rax,4),%r8b
> > +
> > +	.intel_syntax noprefix
> 
> Along the lines of the earlier remark, I wonder whether this repetition of
> everything is really useful to have.

Dropped.

Thanks,
Lili.