Re: [PATCH 4/8] Add tests for APX GPR32 with extend evex prefix

[Jan Beulich <jbeulich@suse.com>]

On 02.11.2023 12:29, Cui, Lili wrote:
> --- a/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
> @@ -1,4 +1,4 @@
> -# Check Illegal 64bit APX_F instructions
> +# Check illegal 64bit APX_F instructions
>  	.text
>  	.arch .noapx_f
>  	test    $0x7, %r17d
> @@ -16,3 +16,195 @@
>  	xsaveopt64 (%r16, %r31)
>  	xsavec (%r16, %rbx)
>  	xsavec64 (%r16, %r31)
> +#SSE
> +	phaddw          (%r17),%xmm0
> +	phaddd          (%r17),%xmm0
> +	phaddsw         (%r17),%xmm0
> +	phsubw          (%r17),%xmm0
> +	pmaddubsw       (%r17),%xmm0
> +	pmulhrsw        (%r17),%xmm0
> +	pshufb          (%r17),%xmm0
> +	psignb          (%r17),%xmm0
> +	psignw          (%r17),%xmm0
> +	psignd          (%r17),%xmm0
> +	palignr $100,(%r17),%xmm6
> +	pabsb          (%r17),%xmm0
> +	pabsw          (%r17),%xmm0
> +	pabsd          (%r17),%xmm0
> +	blendpd $100,(%r18),%xmm6
> +	blendps $100,(%r18),%xmm6
> +	blendvpd %xmm0,(%r19),%xmm6
> +	blendvps %xmm0,(%r19),%xmm6
> +	blendvpd (%r19),%xmm6
> +	blendvps (%r19),%xmm6
> +	dppd $100,(%r20),%xmm6
> +	dpps $100,(%r20),%xmm6
> +	extractps $100,%xmm4,(%r21)
> +	extractps $100,%xmm4,%r21
> +	insertps $100,(%r21),%xmm6
> +	movntdqa (%r21),%xmm4
> +	mpsadbw $100,(%r21),%xmm6
> +	packusdw (%r21),%xmm6
> +	pblendvb %xmm0,(%r22),%xmm6
> +	pblendvb (%r22),%xmm6
> +	pblendw $100,(%r22),%xmm6
> +	pcmpeqq (%r22),%xmm6
> +	pextrb $100,%xmm4,(%r22)
> +	pextrb $100,%xmm4,%r22
> +	pextrw $100,%xmm4,(%r22)
> +	pextrd $100,%xmm4,(%r22)
> +        pextrq $100,%xmm4,(%r22)

Nit: Indentation inconsistency.

> +	phminposuw (%r23),%xmm4
> +	pinsrb $100,%r23,%xmm4
> +	pinsrb $100,(%r23),%xmm4
> +	pinsrd $100, %r23d, %xmm4
> +	pinsrd $100,(%r23),%xmm4
> +	pinsrq $100, %r24, %xmm4
> +	pinsrq $100,(%r24),%xmm4
> +	pmaxsb (%r24),%xmm6
> +	pmaxsd (%r24),%xmm6
> +	pmaxud (%r24),%xmm6
> +	pmaxuw (%r24),%xmm6
> +	pminsb (%r24),%xmm6
> +	pminsd (%r24),%xmm6
> +	pminud (%r24),%xmm6
> +	pminuw (%r24),%xmm6
> +	pmovsxbw (%r24),%xmm4
> +	pmovsxbd (%r24),%xmm4
> +	pmovsxbq (%r24),%xmm4
> +	pmovsxwd (%r24),%xmm4
> +	pmovsxwq (%r24),%xmm4
> +	pmovsxdq (%r24),%xmm4
> +	pmovsxbw (%r24),%xmm4
> +	pmovzxbd (%r24),%xmm4
> +	pmovzxbq (%r24),%xmm4
> +	pmovzxwd (%r24),%xmm4
> +	pmovzxwq (%r24),%xmm4
> +	pmovzxdq (%r24),%xmm4
> +	pmuldq (%r24),%xmm4
> +	pmulld (%r24),%xmm4
> +	roundpd $100,(%r24),%xmm6
> +	roundps $100,(%r24),%xmm6
> +	roundsd $100,(%r24),%xmm6
> +	roundss $100,(%r24),%xmm6
> +	pcmpestri $100,(%r25),%xmm6
> +	pcmpestrm $100,(%r25),%xmm6
> +	pcmpgtq (%r25),%xmm4
> +	pcmpistri $100,(%r25),%xmm6
> +	pcmpistrm $100,(%r25),%xmm6
> +#AES
> +	aesdec (%r26),%xmm6
> +	aesdeclast (%r26),%xmm6
> +	aesenc (%r26),%xmm6
> +	aesenclast (%r26),%xmm6
> +	aesimc (%r26),%xmm6
> +	aeskeygenassist $100,(%r26),%xmm6
> +	pclmulqdq $100,(%r26),%xmm6
> +	pclmullqlqdq (%r26),%xmm6
> +	pclmulhqlqdq (%r26),%xmm6
> +	pclmullqhqdq (%r26),%xmm6
> +	pclmulhqhqdq (%r26),%xmm6
> +#GFNI
> +	gf2p8affineqb $100,(%r26),%xmm6
> +	gf2p8affineinvqb $100,(%r26),%xmm6
> +	gf2p8mulb (%r26),%xmm6
> +#VEX without evex
> +	vblendpd $7,(%r27),%xmm6,%xmm2
> +	vblendpd $7,(%r27),%ymm6,%ymm2
> +	vblendps $7,(%r27),%xmm6,%xmm2
> +	vblendps $7,(%r27),%ymm6,%ymm2
> +	vblendvpd %xmm4,(%r27),%xmm2,%xmm7
> +	vblendvpd %ymm4,(%r27),%ymm2,%ymm7
> +	vblendvps %xmm4,(%r27),%xmm2,%xmm7
> +	vblendvps %ymm4,(%r27),%ymm2,%ymm7
> +	vdppd $7,(%r27),%xmm6,%xmm2
> +	vdpps $7,(%r27),%xmm6,%xmm2
> +	vdpps $7,(%r27),%ymm6,%ymm2
> +	vhaddpd (%r27),%xmm6,%xmm5
> +	vhaddpd (%r27),%ymm6,%ymm5
> +	vhsubps (%r27),%xmm6,%xmm5
> +	vhsubps (%r27),%ymm6,%ymm5
> +	vlddqu (%r27),%xmm4
> +	vlddqu (%r27),%ymm4
> +	vldmxcsr (%r27)

As mentioned before, for this, ...

> +	vmaskmovpd (%r27),%xmm4,%xmm6
> +	vmaskmovpd %xmm4,%xmm6,(%r27)
> +	vmaskmovps (%r27),%xmm4,%xmm6
> +	vmaskmovps %xmm4,%xmm6,(%r27)
> +	vmaskmovpd (%r27),%ymm4,%ymm6
> +	vmaskmovpd %ymm4,%ymm6,(%r27)
> +	vmaskmovps (%r27),%ymm4,%ymm6
> +	vmaskmovps %ymm4,%ymm6,(%r27)	
> +	vmovmskpd %xmm4,%r27d
> +	vmovmskpd %xmm8,%r27d
> +	vmovmskps %xmm4,%r27d
> +	vmovmskps %ymm8,%r27d
> +	vpblendvb %xmm4,(%r27),%xmm2,%xmm7
> +	vpblendvb %ymm4,(%r27),%ymm2,%ymm7
> +	vpblendw $7,(%r27),%xmm6,%xmm2
> +	vpblendw $7,(%r27),%ymm6,%ymm2
> +	vpcmpestri $7,(%r27),%xmm6
> +	vpcmpestrm $7,(%r27),%xmm6
> +	vperm2f128 $7,(%r27),%ymm6,%ymm2
> +	vphaddd (%r27),%xmm6,%xmm7
> +	vphaddsw (%r27),%xmm6,%xmm7
> +	vphaddw (%r27),%xmm6,%xmm7
> +	vphsubd (%r27),%xmm6,%xmm7
> +	vphsubsw (%r27),%xmm6,%xmm7
> +	vphsubw (%r27),%xmm6,%xmm7
> +	vphaddd (%r27),%ymm6,%ymm7
> +	vphaddsw (%r27),%ymm6,%ymm7
> +	vphaddw (%r27),%ymm6,%ymm7
> +	vphsubd (%r27),%ymm6,%ymm7
> +	vphsubsw (%r27),%ymm6,%ymm7
> +	vphsubw (%r27),%ymm6,%ymm7
> +	vphminposuw (%r27),%xmm6
> +	vpmovmskb %xmm4,%r27
> +	vpmovmskb %ymm4,%r27d
> +	vpsignb (%r27),%xmm6,%xmm7
> +	vpsignw (%r27),%xmm6,%xmm7
> +	vpsignd (%r27),%xmm6,%xmm7
> +	vpsignb (%r27),%xmm6,%xmm7
> +	vpsignw (%r27),%xmm6,%xmm7
> +	vpsignd (%r27),%xmm6,%xmm7
> +	vptest (%r27),%xmm6
> +	vptest (%r27),%ymm6
> +	vrcpps (%r27),%xmm6
> +	vrcpps (%r27),%ymm6
> +	vrcpss (%r27),%xmm6,%xmm6
> +	vrsqrtps (%r27),%xmm6
> +	vrsqrtps (%r27),%ymm6
> +	vrsqrtss (%r27),%xmm6,%xmm6
> +	vstmxcsr (%r27)

... this, and ...

> +	vtestps (%r27),%xmm6
> +	vtestps (%r27),%ymm6
> +	vtestpd (%r27),%xmm6
> +	vtestps (%r27),%ymm6
> +	vtestpd (%r27),%ymm6
> +	vpblendd $7,(%r27),%xmm6,%xmm2
> +	vpblendd $7,(%r27),%ymm6,%ymm2
> +	vperm2i128 $7,(%r27),%ymm6,%ymm2
> +	vpmaskmovd (%r27),%xmm4,%xmm6
> +	vpmaskmovd %xmm4,%xmm6,(%r27)
> +	vpmaskmovq (%r27),%xmm4,%xmm6
> +	vpmaskmovq %xmm4,%xmm6,(%r27)
> +	vpmaskmovd (%r27),%ymm4,%ymm6
> +	vpmaskmovd %ymm4,%ymm6,(%r27)
> +	vpmaskmovq (%r27),%ymm4,%ymm6
> +	vpmaskmovq %ymm4,%ymm6,(%r27)
> +	vaesimc (%r27), %xmm3
> +	vaeskeygenassist $7,(%r27),%xmm3
> +	vroundpd $1,(%r24),%xmm6
> +	vroundps $2,(%r24),%xmm6
> +	vroundsd $3,(%r24),%xmm6,%xmm3
> +	vroundss $4,(%r24),%xmm6,%xmm3

... and these four I wonder whether the documentation shouldn't at least
allow room for translating them, for there being functionally equivalent
encodings.

> +	vpcmpistri $100,(%r25),%xmm6
> +	vpcmpistrm $100,(%r25),%xmm6
> +	vpcmpeqb (%r26),%ymm6,%ymm2
> +	vpcmpeqw (%r16),%ymm6,%ymm2
> +	vpcmpeqd (%r26),%ymm6,%ymm2
> +	vpcmpeqq (%r16),%ymm6,%ymm2
> +	vpcmpgtb (%r26),%ymm6,%ymm2
> +	vpcmpgtw (%r16),%ymm6,%ymm2
> +	vpcmpgtd (%r26),%ymm6,%ymm2
> +	vpcmpgtq (%r16),%ymm6,%ymm2

As an overall remark to this (and perhaps similar) test(s): It would be
nice if there was some consistent sorting criteria applied throughout
the test as whole or (here) the sub-sections (validly grouped by
category). Without that it's needlessly hard to spot any omissions.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-promote-inval.l
> @@ -0,0 +1,16 @@
> +.*: Assembler messages:
> +.*:4: Error: `movbe' is not supported on `x86_64.nomovbe'
> +.*:5: Error: `movbe' is not supported on `x86_64.nomovbe'
> +.*:7: Error: `invept' is not supported on `x86_64.nomovbe.noept'
> +.*:8: Error: `invept' is not supported on `x86_64.nomovbe.noept'
> +.*:10: Error: `kmovq' is not supported on `x86_64.nomovbe.noept.noavx512bw'
> +.*:11: Error: `kmovq' is not supported on `x86_64.nomovbe.noept.noavx512bw'
> +.*:13: Error: `kmovb' is not supported on `x86_64.nomovbe.noept.noavx512bw.noavx512dq'
> +.*:14: Error: `kmovb' is not supported on `x86_64.nomovbe.noept.noavx512bw.noavx512dq'
> +.*:16: Error: `kmovw' is not supported on `x86_64.nomovbe.noept.noavx512bw.noavx512dq.noavx512f'
> +.*:17: Error: `kmovw' is not supported on `x86_64.nomovbe.noept.noavx512bw.noavx512dq.noavx512f'

Can the irrelevant middle parts of these .no* expecations please be omitted?
The construction of these strings is in need of improvement, and it would be
nice if testcases where the precise string doesn't matter would then not
need touching. (This is a more general principle: Testcase expectations
would better be only as specific as needed for what is under test. Certainly
multiple aspects may be tested in one go, but quite commonly expecations are
needlessly strict, and hence needlessly prone to breaking when unrelated
changes are made somewhere in the code.)

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-promote-inval.s
> @@ -0,0 +1,17 @@
> +# Check illegal 64bit APX EVEX promoted instructions
> +	.text
> +	.arch .nomovbe
> +	movbe (%r16), %r17
> +	movbe (%rax), %rcx
> +	.arch .noept
> +	invept (%r16), %r17
> +	invept (%rax), %rcx
> +	.arch .noavx512bw
> +	kmovq %k1, (%r16)
> +	kmovq %k1, (%r8)
> +	.arch .noavx512dq
> +	kmovb %k1, %r16d
> +	kmovb %k1, %r8d
> +	.arch .noavx512f
> +	kmovw %k1, %r16d
> +	kmovw %k1, %r8d

What about BMI/BMI2 insns? Or AMX ones? (I surely missed further groups.)

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> @@ -0,0 +1,29 @@
> +# Check Illegal prefix for 64bit EVEX-promoted instructions
> +
> +        .allow_index_reg
> +        .text
> +_start:
> +        #movbe %r18w,%ax set EVEX.pp = f3 (illegal value).
> +        .byte 0x62, 0xfc, 0x7e, 0x08, 0x60, 0xc2
> +        .byte 0xff, 0xff
> +        #movbe %r18w,%ax set EVEX.pp = f2 (illegal value).
> +        .byte 0x62, 0xfc, 0x7f, 0x08, 0x60, 0xc2
> +        .byte 0xff, 0xff
> +        #VSIB vpgatherqq 0x7b(%rbp,%zmm17,8),%zmm16{%k1} set EVEX.P[10] == 0
> +	#(illegal value).
> +        .byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd, 0x7b, 0x00, 0x00, 0x00
> +        .byte 0xff
> +        #EVEX_MAP4 movbe %r18w,%ax set EVEX.mm == b01 (illegal value).
> +	.byte 0x62, 0xfd, 0x7d, 0x08, 0x60, 0xc2
> +        .byte 0xff, 0xff
> +        #EVEX_MAP4 movbe %r18w,%ax set EVEX.aa(P[17:16]) == b01 (illegal value).
> +	.byte 0x62, 0xfd, 0x7d, 0x09, 0x60, 0xc2
> +        .byte 0xff, 0xff
> +        #EVEX_MAP4 movbe %r18w,%ax set EVEX.zL'L == b001 (illegal value).
> +	.byte 0x62, 0xfd, 0x7d, 0x28, 0x60, 0xc2
> +        .byte 0xff, 0xff
> +        #EVEX from VEX enqcmd 0x123(%r31,%rax,4),%r31 EVEX.P[17:16] == 1 (illegal value).
> +        .byte 0x62, 0x4c, 0x7f, 0x09, 0xf8, 0xbc, 0x87, 0x23, 0x01, 0x00, 0x00
> +        .byte 0xff
> +        #EVEX from VEX enqcmd 0x123(%r31,%rax,4),%r31 EVEX.P[23:22] == 1 (illegal value).
> +        .byte 0x62, 0x4c, 0x7f, 0x28, 0xf8, 0xbc, 0x87, 0x23, 0x01, 0x00, 0x00

I suspect at least some of these can be expressed via .insn, which would
greatly help readability (i.e. recognizing what is actually being done,
and what's expected-wrong about it).

Also - nit - there are again indentation inconsistencies here.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted.s
> @@ -0,0 +1,322 @@
> +# Check 64bit APX_F EVEX-Promoted instructions.
> +
> +	.text
> +_start:
>[...]
> +.intel_syntax noprefix

Didn't you say you corrected directive indentation throughout the series?

> +	aadd	DWORD PTR [r31+rax*4+0x123],r25d
> +	aadd	QWORD PTR [r31+rax*4+0x123],r31
> +	aand	DWORD PTR [r31+rax*4+0x123],r25d
> +	aand	QWORD PTR [r31+rax*4+0x123],r31
> +	aesdec128kl	xmm22,[r31+rax*4+0x123]
> +	aesdec256kl	xmm22,[r31+rax*4+0x123]
> +	aesdecwide128kl	[r31+rax*4+0x123]
> +	aesdecwide256kl	[r31+rax*4+0x123]
> +	aesenc128kl	xmm22,[r31+rax*4+0x123]
> +	aesenc256kl	xmm22,[r31+rax*4+0x123]
> +	aesencwide128kl	[r31+rax*4+0x123]
> +	aesencwide256kl	[r31+rax*4+0x123]
> +	aor	DWORD PTR [r31+rax*4+0x123],r25d
> +	aor	QWORD PTR [r31+rax*4+0x123],r31
> +	axor	DWORD PTR [r31+rax*4+0x123],r25d
> +	axor	QWORD PTR [r31+rax*4+0x123],r31
> +	bextr	r10d,edx,r25d
> +	bextr	edx,DWORD PTR [r31+rax*4+0x123],r25d
> +	bextr	r11,r15,r31
> +	bextr	r15,QWORD PTR [r31+rax*4+0x123],r31

Going just down to here (it extends throughout the Intel syntax part):
Can there please also be cases where the xxx PTR is omitted from the
memory operands? That doesn't mean there always need to be both forms,
but there should be a fair mix. (I notice you have one such example
with INVPCID below.)

>[...]
> +	crc32	r22,r31
> +	crc32	r22,QWORD PTR [r31]
> +	crc32	r17,r19b
> +	crc32	r21d,r19b
> +	crc32	ebx,BYTE PTR [r19]
> +	crc32	r23d,r31d
> +	crc32	r23d,DWORD PTR [r31]
> +	crc32	r21d,r31w
> +	crc32	r21d,WORD PTR [r31]
> +	crc32	r18,rax

These could do with moving up, since otherwise things look to be sorted
alphabetically here. But seeing these also reminds me that the noreg64
test also needs extending, to cover these new forms (handled by separate
templates).

> +	kmovb	k5,k3

This (and its siblings) doesn't belong, here, does it? It continues to
be VEX-encoded.

> --- a/gas/testsuite/gas/i386/x86-64.exp
> +++ b/gas/testsuite/gas/i386/x86-64.exp
> @@ -360,8 +360,13 @@ run_dump_test "x86-64-avx512f-rcigrne-intel"
>  run_dump_test "x86-64-avx512f-rcigrne"
>  run_dump_test "x86-64-avx512f-rcigru-intel"
>  run_dump_test "x86-64-avx512f-rcigru"
> -run_list_test "x86-64-apx-egpr-inval" "-al"
> +run_list_test "x86-64-apx-egpr-inval"

This should be put in its final shape right in patch 1; no need to touch
it here again. (Else you'd need to mention the change in the ChangeLog
entry.)

Jan