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

[Jan Beulich <jbeulich@suse.com>]

On 25.12.2023 14:40, Cui, Lili wrote:
>> 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. 

This replacement should have been introduced in this series, as asked for
more than once. And, H.J., I think you shouldn't have approved this patch
(and more generally any one) with unaddressed review comments.

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

This is not addressing my comment. Iirc all bit in the EVEX prefix have
names now in SDM plus APX spec, and hence they should preferably be
referred to by their names rather than their bit positions. Plus there's
still no justification for using .byte instead of .insn.

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

What is "that" here? And how can anything with operand mask applied be
VEX? The more that .insn's operand says EVEX? Here you want to set _only_
EVEX.z, without EVEX.aaa being non-zero.

Jan