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

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

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

Re-sorted for each group.

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

Done.

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

We don’t want to list all the instructions here, just a few representatives.

> > --- /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 ldtilecfg 0x123(%r31,%rax,4),%r31 EVEX.P[17:16](EVEX.aa) == 1 (illegal value).
        .byte 0x62, 0xda, 0x7c, 0x09, 0x49, 0x84, 0x87, 0x23, 0x01, 0x00, 0x00
        #EVEX from VEX ldtilecfg 0x123(%r31,%rax,4),%r31 EVEX.P[22:21](EVEX.L’L) == 1 (illegal value).
        .byte 0x62, 0xda, 0x7c, 0x28, 0x49, 0x84, 0x87, 0x23, 0x01, 0x00, 0x00
        #EVEX from VEX ldtilecfg 0x123(%r31,%rax,4),%r31 EVEX.P[20](EVEX.b) == 1 (illegal value).
        .byte 0x62, 0xda, 0x7c, 0x18, 0x49, 0x84, 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).
> 

Update test cases.
I try to express the first case using .insn. I can't find a way to express EVEX.P[3:2] == 11, do you have any ideas?

0x62, 0xfc  ---> EVEX.P[3:2] of normal EVEX must be 00.

> Also - nit - there are again indentation inconsistencies here.

Done.

> > --- /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?
> 

Done.

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

Changed.

> >[...]
> > +	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).
> 

I'm confused here about adding crc test case in noreg64.s, could you elaborate on what testcase you want to add?

        pfx crc32       (%rax), %eax
        pfx16 crc32     (%rax), %rax
+       pfx crc32       (%r31),%r21d   ---> data size prefix invalid with `crc32'
+       pfx crc32       (%r31),%r21     ---> data size prefix invalid with `crc32'

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

Done.

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

Done.

Thanks,
Lili.