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

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

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

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)


>  	encoding_evex,
>  	encoding_evex512,
>  	encoding_error
> @@ -1887,7 +1885,7 @@ static INLINE bool need_evex_encoding (c  {
>    return i.encoding == encoding_evex
>  	|| i.encoding == encoding_evex512
> -	|| (t->opcode_modifier.vex && i.has_egpr)
> +	|| (t->opcode_modifier.vex && i.encoding == encoding_egpr)
>  	|| i.mask.reg;
>  }
> 
> +.*: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'
>  #pass
> --- 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.

> +	{vex} andn %r16, %r16, %r16

Thanks,
Lili.