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

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

> On 19.12.2023 13:12, 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
> > @@ -16,3 +16,194 @@
> > +	vroundpd $1,(%r24),%xmm6
> > +	vroundps $2,(%r24),%xmm6
> > +	vroundsd $3,(%r24),%xmm6,%xmm3
> > +	vroundss $4,(%r24),%xmm6,%xmm3
> 
> These are still here, when they can be expressed.

I think we should keep it until we complete a reasonably equivalent replacement. 

> 
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> > @@ -0,0 +1,34 @@
> > +# Check Illegal prefix for 64bit EVEX-promoted instructions
> > +
> > +        .allow_index_reg
> > +        .text
> > +_start:
> > +	#movbe %r23w,%ax set EVEX.pp = f3 (illegal value).
> > +	.insn EVEX.L0.f3.M12.W0 0x60, %di, %ax
> > +	#movbe %r23w,%ax set EVEX.pp = f2 (illegal value).
> > +	.insn EVEX.L0.f2.M12.W0 0x60, %di, %ax
> > +	#VSIB vpgatherqq (%rbp,%zmm17,8),%zmm16{%k1} set EVEX.P[10]
> == 0
> > +	#(illegal value).
> > +	.byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd
> > +	.byte 0xff
> 
> This one's still using .byte and still referencing P[10] in the comment.
> If that's really unavoidable, then - as elsewhere - the description of the patch
> could (and should) provide the reason. And there continue to be no
> separating blank lines, making it as hard as before to find ones way through all
> of this.
> 

Copied from the previous comments,
------------------------------------------------------------------------
>>> +	#(illegal value).
>>> +	.byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd, 0x7b, 0x00, 0x00, 0x00
>>> +	.byte 0xff
>>
>> For the purpose of this test (whatever P[10] again is) you don't need 
>> a 32-bit displacement, do you? Shorter is (almost always) better in such tests.
>>
> 
> P[10] is a fixed value, in normal EVEX format we don't use this bit.  Dropped 0x7b.
------------------------------------------------------------------------

> > +	#EVEX_MAP4 movbe %r23w,%ax set EVEX.mm == 0b01 (illegal value).
> > +	.insn EVEX.L0.66.M13.W0 0x60, %di, %ax
> > +	#EVEX_MAP4 movbe %r23w,%ax set EVEX.a1a0 (P[17:16]) == 0b01
> 
> See my earlier comment about a1a0. Once again - please can you use SDM
> terms as much as possible, and prefer descriptive names over non- descriptive
> ones?
> 
Done.

> > +        #(illegal value).
> 
> ??? At the very least indentation is bogus here, but as to this particular
> comment (recurring elsewhere) - isn't the entire test about illegal values?
> 
Removed them.

> > +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax{%k1}
> > +	#EVEX_MAP4 movbe %r18w,%ax set EVEX.L'L == 0b01 (illegal value).
> > +	.insn EVEX.L1.66.M12.W0 0x60, %di, %ax
> > +	#EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1 (illegal value).
> > +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k7}{z}
> > +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[17:16](EVEX.aa)
> == 0b01
> > +	#(illegal value).
> > +	.insn EVEX.L0.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx{%k1}
> > +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%ecx EVEX.P[22:21](EVEX.L’L)
> == 0b01
> > +	#(illegal value).
> > +	.insn EVEX.L1.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx
> > +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) ==
> 0b1
> > +	#(illegal value).
> > +        .insn EVEX.L0.NP.0f38.W0 0xf5, %rax, (%rax,%rbx), %rcx
> > +{%k7}{z}
> 
> Isn't {%k7} alone rendering the encoding invalid, which was checked for above
> already? Also - bogus indentation again.
>

That is for legacy, this is for vex. Adjusted indentation and added separating blank lines for all insns.

> > +	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[20](EVEX.b) ==
> 0b1
> > +	#(illegal value).
> > +	.insn EVEX.L0.NP.0f38.W0 0xf5, %rax ,(%rax,%rbx){1to8}, %rcx
> 
> 64-bit register operands with .W0 are kind of okay, but still odd to see. Note
> however the one misplaced comma here.
> 
Changed to:

        #EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[20](EVEX.b) == 0b1
        .insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx){1to8}, %rcx

Thanks,
Lili.