[PATCH 2/4] x86/APX: respect {vex}/{vex3}

[Jan Beulich]

On 18.02.2024 08:55, Cui, Lili wrote:
>> Even when an EVEX encoding is available, use of such a prefix ought to be
>> respected (resulting in an error) rather than ignored. As requested during
>> review already, introduce a new encoding enumerator to record use of eGPR-
>> s, and update state transitions accordingly.
>>
> 
> Yes, we have such issue for dual VEX/EVEX templates.
> 
>> The optimize_encoding() change also addresses an internal assembler error
>> that was previously raised when respective memory operands used eGPR-s for
>> addressing.
>>
>> While this results in a change of diagnostic issued for VEX-encoded insns, the
>> new one is at least no worse than the prior one.
>> ---
>> Question is whether for the state transitions we want to introduce a couple of
>> helper functions: check_register() has duplicates each of what
>> RC_SAE_specifier() and check_VecOperations() also do.
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -439,9 +439,6 @@ struct _i386_insn
>>      /* Prefer the REX2 prefix in encoding.  */
>>      bool rex2_encoding;
>>
>> -    /* Need to use an Egpr capable encoding (REX2 or EVEX).  */
>> -    bool has_egpr;
>> -
>>      /* Disable instruction size optimization.  */
>>      bool no_optimize;
>>
>> @@ -451,6 +448,7 @@ struct _i386_insn
>>  	encoding_default = 0,
>>  	encoding_vex,
>>  	encoding_vex3,
>> +	encoding_egpr, /* REX2 or EVEX.  */
> 
> I find it difficult to understand putting egpr here.  Although this area can be further optimized, it is difficult to say that this solution is clearer than the current one.
> 
> 1. We have separated vex/evex and rex/rex2, and put the state containing both rex2 and evex in vex/evex encoding, so that the logic becomes confusing.
> 2. In this enumeration, each enumeration represents an encoding format, and only encoding_egpr describes the register of the operand.

I don't view it like this: This enumerator indicates "need an encoding
which can represent eGPR-s, i.e. REX2 or EVEX". To me encoding_rex2_or_evex
would be pretty clearly worse a name for it.

> 3. encoding_egpr is not the final encoding expression, but an intermediate state that ultimately needs to be converted into other expressions of the same level.

Not much different from encoding_evex512.

> If this patch just wants to report an error to vex prefix, maybe we could handle it like this. Or create a separate branch.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -8322,7 +8322,8 @@ VEX_check_encoding (const insn_template *t)
>        return 0;
>      }
> 
> -  if (!t->opcode_modifier.vex)
> +  if (!t->opcode_modifier.vex
> +      || ((i.vec_encoding == vex_encoding_vex) && i.has_egpr))
>      {
>        /* This instruction template doesn't have VEX prefix.  */
>        if (i.vec_encoding != vex_encoding_default)

That's indeed a possibility, I think. Yet already when reviewing the
original work of yours I indicated that I'd like the encoding
restrictions all be represented by a single enum. To me it is more
difficult to follow when there are two separate entities which need
to be consulted in order to reflect all constraints.

>> +.*:211: Error: no VEX/XOP encoding for `and'
>> +.*:212: Error: no VEX/XOP encoding for `and'
>> +.*:213: Error: .* `and'
>> +.*:214: Error: no VEX/XOP encoding for `and'
>> +.*:215: Error: no VEX/XOP encoding for `and'
>> +.*:216: Error: .* `and'
>> +.*:219: Error: .* `andn'

Please pay attention to the gap in line numbers here; that ...

>> --- a/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
>> +++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.s
>> @@ -207,3 +207,13 @@
>>  	vtestpd (%r27),%ymm6
>>  	vtestps (%r27),%xmm6
>>  	vtestps (%r27),%ymm6
>> +# {vex}
>> +	{vex} and %eax, %eax
>> +	{vex} and %r8, %r8
>> +	{vex} and %r16, %r16
>> +	{vex} and %eax, %eax, %eax
>> +	{vex} and %r8, %r8, %r8
>> +	{vex} and %r16, %r16, %r16
>> +	{vex} andn %eax, %eax, %eax
>> +	{vex} andn %r8, %r8, %r8
> 
> These two test cases are valid.

... reflects exactly this fact.

Jan