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

Message ID 8a7e7a43-37d4-425c-8166-6ba8b758f0f4@suse.com
State Unresolved
Headers
Series x86/APX: misc adjustments |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Jan Beulich Feb. 16, 2024, 9:58 a.m. UTC
  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.

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.
  

Comments

Cui, Lili Feb. 18, 2024, 7:55 a.m. UTC | #1
> 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.
  
Jan Beulich Feb. 19, 2024, 8 a.m. UTC | #2
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
  
Cui, Lili Feb. 20, 2024, 10:12 a.m. UTC | #3
> 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.
> 

I think the essential problem is that it's strange to put it in this enumeration.

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

Yes, encoding_evex512 is also an intermediate state, at least it chooses one of the states in this enumeration. 

    /* Prefer the REX byte in encoding.  */
    bool rex_encoding;

    /* Prefer the REX2 prefix in encoding.  */
    bool rex2_encoding;

    /* Need to use an Egpr capable encoding (REX2 or EVEX).  */
    bool has_egpr;

    /* How to encode vector instructions.  */
    enum
      {
        vex_encoding_default = 0,
        vex_encoding_vex,
        vex_encoding_vex3,
        vex_encoding_evex,
        vex_encoding_evex512,
        vex_encoding_error
      } vec_encoding;


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

Egpr does conflict with the existing architecture, making implementation awkward.

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

Normally we don't put valid test cases into invalid test case file, right?

Thanks,
Lili.
  
Jan Beulich Feb. 20, 2024, 10:30 a.m. UTC | #4
On 20.02.2024 11:12, Cui, Lili wrote:
>> On 18.02.2024 08:55, 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
>>>> @@ -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.
>>
> 
> Normally we don't put valid test cases into invalid test case file, right?

Depends, I would say (and I think you'll find other examples). I view it as
pretty relevant here.

Jan
  
Michael Matz Feb. 20, 2024, 3:59 p.m. UTC | #5
Hello,

On Tue, 20 Feb 2024, Jan Beulich wrote:

> On 20.02.2024 11:12, Cui, Lili wrote:
> >> On 18.02.2024 08:55, 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
> >>>> @@ -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.
> >>
> > 
> > Normally we don't put valid test cases into invalid test case file, right?
> 
> Depends, I would say (and I think you'll find other examples). I view it as
> pretty relevant here.

I think the test filename should be different then, also for other cases 
where this is the case.  That, or at least a comment next to the insns 
that those are _not_ invalid (referring to the .d file to see that is not 
self-describing).  Without any other indication I'd say instructions in a 
file named "*inval.s" should _all_ be invalid, always.


Ciao,
Michael.
  
H.J. Lu Feb. 20, 2024, 4:52 p.m. UTC | #6
On Tue, Feb 20, 2024 at 8:00 AM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Tue, 20 Feb 2024, Jan Beulich wrote:
>
> > On 20.02.2024 11:12, Cui, Lili wrote:
> > >> On 18.02.2024 08:55, 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
> > >>>> @@ -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.
> > >>
> > >
> > > Normally we don't put valid test cases into invalid test case file, right?
> >
> > Depends, I would say (and I think you'll find other examples). I view it as
> > pretty relevant here.
>
> I think the test filename should be different then, also for other cases
> where this is the case.  That, or at least a comment next to the insns
> that those are _not_ invalid (referring to the .d file to see that is not
> self-describing).  Without any other indication I'd say instructions in a
> file named "*inval.s" should _all_ be invalid, always.

Agreed.
  

Patch

--- 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.  */
 	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;
 }
 
@@ -2489,7 +2487,8 @@  static INLINE int
 fits_in_imm4 (offsetT num)
 {
   /* Despite the name, check for imm3 if we're dealing with EVEX.  */
-  return (num & (i.encoding != encoding_evex ? 0xf : 7)) == num;
+  return (num & (i.encoding != encoding_evex
+		 && i.encoding != encoding_egpr ? 0xf : 7)) == num;
 }
 
 static i386_operand_type
@@ -4837,6 +4836,7 @@  optimize_encoding (void)
 	  }
     }
   else if (i.encoding != encoding_evex
+	   && i.encoding != encoding_egpr
 	   && !i.types[0].bitfield.zmmword
 	   && !i.types[1].bitfield.zmmword
 	   && !i.mask.reg
@@ -6839,10 +6839,13 @@  md_assemble (char *line)
   if (optimize && !i.no_optimize && i.tm.opcode_modifier.optimize)
     optimize_encoding ();
 
-  /* Past optimization there's no need to distinguish encoding_evex and
-     encoding_evex512 anymore.  */
+  /* Past optimization there's no need to distinguish encoding_evex,
+     encoding_evex512, and encoding_egpr anymore.  */
   if (i.encoding == encoding_evex512)
     i.encoding = encoding_evex;
+  else if (i.encoding == encoding_egpr)
+    i.encoding = is_any_vex_encoding (&i.tm) ? encoding_evex
+					     : encoding_default;
 
   if (use_unaligned_vector_move)
     encode_with_unaligned_vector_move ();
@@ -8277,27 +8280,42 @@  VEX_check_encoding (const insn_template
       return 1;
     }
 
-  if (i.encoding == encoding_evex
-      || i.encoding == encoding_evex512)
+  switch (i.encoding)
     {
+    case encoding_default:
+      break;
+
+    case encoding_vex:
+    case encoding_vex3:
+      /* This instruction must be encoded with VEX prefix.  */
+      if (!t->opcode_modifier.vex)
+	{
+	  i.error = no_vex_encoding;
+	  return 1;
+	}
+      break;
+
+    case encoding_evex:
+    case encoding_evex512:
       /* This instruction must be encoded with EVEX prefix.  */
       if (!t->opcode_modifier.evex)
 	{
 	  i.error = no_evex_encoding;
 	  return 1;
 	}
-      return 0;
-    }
+      break;
 
-  if (!t->opcode_modifier.vex)
-    {
-      /* This instruction template doesn't have VEX prefix.  */
-      if (i.encoding != encoding_default)
+    case encoding_egpr:
+      /* This instruction must be encoded with REX2 or EVEX prefix.  */
+      if (t->opcode_modifier.vex && !t->opcode_modifier.evex)
 	{
-	  i.error = no_vex_encoding;
+	  i.error = no_evex_encoding;
 	  return 1;
 	}
-      return 0;
+      break;
+
+    default:
+      abort ();
     }
 
   return 0;
@@ -12896,6 +12914,19 @@  s_insn (int dummy ATTRIBUTE_UNUSED)
       if (i.encoding == encoding_evex512)
 	i.encoding = encoding_evex;
 
+      if (i.encoding == encoding_egpr)
+	{
+	  if (vex || xop)
+	    {
+	      as_bad (_("eGPR use conflicts with encoding specifier"));
+	      goto done;
+	    }
+	  if (evex)
+	    i.encoding = encoding_evex;
+	  else
+	    i.encoding = encoding_default;
+	}
+
       /* Are we to emit ModR/M encoding?  */
       if (!i.short_form
 	  && (i.mem_operands
@@ -13340,11 +13371,18 @@  RC_SAE_specifier (const char *pstr)
 	      return NULL;
 	    }
 
-	  if (i.encoding == encoding_default)
-	    i.encoding = encoding_evex512;
-	  else if (i.encoding != encoding_evex
-		   && i.encoding != encoding_evex512)
-	    return NULL;
+	  switch (i.encoding)
+	    {
+	    case encoding_default:
+	    case encoding_egpr:
+	      i.encoding = encoding_evex512;
+	      break;
+	    case encoding_evex:
+	    case encoding_evex512:
+	      break;
+	    default:
+	      return NULL;
+	    }
 
 	  i.rounding.type = RC_NamesTable[j].type;
 
@@ -13405,11 +13443,18 @@  check_VecOperations (char *op_string)
 		}
 	      op_string++;
 
-	      if (i.encoding == encoding_default)
-		i.encoding = encoding_evex;
-	      else if (i.encoding != encoding_evex
-		       && i.encoding != encoding_evex512)
-		goto unknown_vec_op;
+	      switch (i.encoding)
+		{
+		case encoding_default:
+		case encoding_egpr:
+		  i.encoding = encoding_evex;
+		  break;
+		case encoding_evex:
+		case encoding_evex512:
+		  break;
+		default:
+		  goto unknown_vec_op;
+		}
 
 	      i.broadcast.type = bcst_type;
 	      i.broadcast.operand = this_operand;
@@ -15676,11 +15721,19 @@  static bool check_register (const reg_en
       if (vector_size < VSZ512)
 	return false;
 
-      if (i.encoding == encoding_default)
-	i.encoding = encoding_evex512;
-      else if (i.encoding != encoding_evex
-	       && i.encoding != encoding_evex512)
-	i.encoding = encoding_error;
+      switch (i.encoding)
+	{
+	case encoding_default:
+	case encoding_egpr:
+	  i.encoding = encoding_evex512;
+	  break;
+	case encoding_evex:
+	case encoding_evex512:
+	  break;
+	default:
+	  i.encoding = encoding_error;
+	  break;
+	}
     }
 
   if (vector_size < VSZ256 && r->reg_type.bitfield.ymmword)
@@ -15706,11 +15759,19 @@  static bool check_register (const reg_en
 	  || flag_code != CODE_64BIT)
 	return false;
 
-      if (i.encoding == encoding_default
-	  || i.encoding == encoding_evex512)
-	i.encoding = encoding_evex;
-      else if (i.encoding != encoding_evex)
-	i.encoding = encoding_error;
+      switch (i.encoding)
+	{
+	  case encoding_default:
+	  case encoding_egpr:
+	  case encoding_evex512:
+	    i.encoding = encoding_evex;
+	    break;
+	  case encoding_evex:
+	    break;
+	  default:
+	    i.encoding = encoding_error;
+	    break;
+	}
     }
 
   if (r->reg_flags & RegRex2)
@@ -15719,7 +15780,19 @@  static bool check_register (const reg_en
 	  || flag_code != CODE_64BIT)
 	return false;
 
-      i.has_egpr = true;
+      switch (i.encoding)
+	{
+	case encoding_default:
+	  i.encoding = encoding_egpr;
+	  break;
+	case encoding_egpr:
+	case encoding_evex:
+	case encoding_evex512:
+	  break;
+	default:
+	  i.encoding = encoding_error;
+	  break;
+	}
     }
 
   if (((r->reg_flags & (RegRex64 | RegRex)) || r->reg_type.bitfield.qword)
--- a/gas/config/tc-i386-intel.c
+++ b/gas/config/tc-i386-intel.c
@@ -209,11 +209,18 @@  operatorT i386_operator (const char *nam
 	      || i386_types[j].sz[0] > 8
 	      || (i386_types[j].sz[0] & (i386_types[j].sz[0] - 1)))
 	    return O_illegal;
-	  if (i.encoding == encoding_default)
-	    i.encoding = encoding_evex;
-	  else if (i.encoding != encoding_evex
-		   && i.encoding != encoding_evex512)
-	    return O_illegal;
+	  switch (i.encoding)
+	    {
+	    case encoding_default:
+	    case encoding_egpr:
+	      i.encoding = encoding_evex;
+	      break;
+	    case encoding_evex:
+	    case encoding_evex512:
+	      break;
+	    default:
+	      return O_illegal;
+	    }
 	  if (!i.broadcast.bytes && !i.broadcast.type)
 	    {
 	      i.broadcast.bytes = i386_types[j].sz[0];
--- a/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.l
+++ b/gas/testsuite/gas/i386/x86-64-apx-egpr-inval.l
@@ -101,102 +101,109 @@ 
 .*:108: Error: extended GPR cannot be used as base/index for `gf2p8affineinvqb'
 .*:109: Error: extended GPR cannot be used as base/index for `gf2p8affineqb'
 .*:110: Error: extended GPR cannot be used as base/index for `gf2p8mulb'
-.*:112: Error: extended GPR cannot be used as base/index for `vaesimc'
-.*:113: Error: extended GPR cannot be used as base/index for `vaeskeygenassist'
-.*:114: Error: extended GPR cannot be used as base/index for `vblendpd'
-.*:115: Error: extended GPR cannot be used as base/index for `vblendpd'
-.*:116: Error: extended GPR cannot be used as base/index for `vblendps'
-.*:117: Error: extended GPR cannot be used as base/index for `vblendps'
-.*:118: Error: extended GPR cannot be used as base/index for `vblendvpd'
-.*:119: Error: extended GPR cannot be used as base/index for `vblendvpd'
-.*:120: Error: extended GPR cannot be used as base/index for `vblendvps'
-.*:121: Error: extended GPR cannot be used as base/index for `vblendvps'
-.*:122: Error: extended GPR cannot be used as base/index for `vdppd'
-.*:123: Error: extended GPR cannot be used as base/index for `vdpps'
-.*:124: Error: extended GPR cannot be used as base/index for `vdpps'
-.*:125: Error: extended GPR cannot be used as base/index for `vhaddpd'
-.*:126: Error: extended GPR cannot be used as base/index for `vhaddpd'
-.*:127: Error: extended GPR cannot be used as base/index for `vhsubps'
-.*:128: Error: extended GPR cannot be used as base/index for `vhsubps'
-.*:129: Error: extended GPR cannot be used as base/index for `vlddqu'
-.*:130: Error: extended GPR cannot be used as base/index for `vlddqu'
-.*:131: Error: extended GPR cannot be used as base/index for `vldmxcsr'
-.*:132: Error: extended GPR cannot be used as base/index for `vmaskmovpd'
-.*:133: Error: extended GPR cannot be used as base/index for `vmaskmovpd'
-.*:134: Error: extended GPR cannot be used as base/index for `vmaskmovpd'
-.*:135: Error: extended GPR cannot be used as base/index for `vmaskmovpd'
-.*:136: Error: extended GPR cannot be used as base/index for `vmaskmovps'
-.*:137: Error: extended GPR cannot be used as base/index for `vmaskmovps'
-.*:138: Error: extended GPR cannot be used as base/index for `vmaskmovps'
-.*:139: Error: extended GPR cannot be used as base/index for `vmaskmovps'
-.*:140: Error: register type mismatch for `vmovmskpd'
-.*:141: Error: register type mismatch for `vmovmskpd'
-.*:142: Error: register type mismatch for `vmovmskps'
-.*:143: Error: register type mismatch for `vmovmskps'
-.*:144: Error: extended GPR cannot be used as base/index for `vpblendd'
-.*:145: Error: extended GPR cannot be used as base/index for `vpblendd'
-.*:146: Error: extended GPR cannot be used as base/index for `vpblendvb'
-.*:147: Error: extended GPR cannot be used as base/index for `vpblendvb'
-.*:148: Error: extended GPR cannot be used as base/index for `vpblendw'
-.*:149: Error: extended GPR cannot be used as base/index for `vpblendw'
-.*:150: Error: extended GPR cannot be used as base/index for `vpcmpeqb'
-.*:151: Error: extended GPR cannot be used as base/index for `vpcmpeqd'
-.*:152: Error: extended GPR cannot be used as base/index for `vpcmpeqq'
-.*:153: Error: extended GPR cannot be used as base/index for `vpcmpeqw'
-.*:154: Error: extended GPR cannot be used as base/index for `vpcmpestri'
-.*:155: Error: extended GPR cannot be used as base/index for `vpcmpestrm'
-.*:156: Error: extended GPR cannot be used as base/index for `vpcmpgtb'
-.*:157: Error: extended GPR cannot be used as base/index for `vpcmpgtd'
-.*:158: Error: extended GPR cannot be used as base/index for `vpcmpgtq'
-.*:159: Error: extended GPR cannot be used as base/index for `vpcmpgtw'
-.*:160: Error: extended GPR cannot be used as base/index for `vpcmpistri'
-.*:161: Error: extended GPR cannot be used as base/index for `vpcmpistrm'
-.*:162: Error: extended GPR cannot be used as base/index for `vperm2f128'
-.*:163: Error: extended GPR cannot be used as base/index for `vperm2i128'
-.*:164: Error: extended GPR cannot be used as base/index for `vphaddd'
-.*:165: Error: extended GPR cannot be used as base/index for `vphaddd'
-.*:166: Error: extended GPR cannot be used as base/index for `vphaddsw'
-.*:167: Error: extended GPR cannot be used as base/index for `vphaddsw'
-.*:168: Error: extended GPR cannot be used as base/index for `vphaddw'
-.*:169: Error: extended GPR cannot be used as base/index for `vphaddw'
-.*:170: Error: extended GPR cannot be used as base/index for `vphminposuw'
-.*:171: Error: extended GPR cannot be used as base/index for `vphsubd'
-.*:172: Error: extended GPR cannot be used as base/index for `vphsubd'
-.*:173: Error: extended GPR cannot be used as base/index for `vphsubsw'
-.*:174: Error: extended GPR cannot be used as base/index for `vphsubsw'
-.*:175: Error: extended GPR cannot be used as base/index for `vphsubw'
-.*:176: Error: extended GPR cannot be used as base/index for `vphsubw'
-.*:177: Error: extended GPR cannot be used as base/index for `vpmaskmovd'
-.*:178: Error: extended GPR cannot be used as base/index for `vpmaskmovd'
-.*:179: Error: extended GPR cannot be used as base/index for `vpmaskmovd'
-.*:180: Error: extended GPR cannot be used as base/index for `vpmaskmovd'
-.*:181: Error: extended GPR cannot be used as base/index for `vpmaskmovq'
-.*:182: Error: extended GPR cannot be used as base/index for `vpmaskmovq'
-.*:183: Error: extended GPR cannot be used as base/index for `vpmaskmovq'
-.*:184: Error: extended GPR cannot be used as base/index for `vpmaskmovq'
-.*:185: Error: register type mismatch for `vpmovmskb'
-.*:186: Error: register type mismatch for `vpmovmskb'
-.*:187: Error: extended GPR cannot be used as base/index for `vpsignb'
-.*:188: Error: extended GPR cannot be used as base/index for `vpsignb'
-.*:189: Error: extended GPR cannot be used as base/index for `vpsignd'
-.*:190: Error: extended GPR cannot be used as base/index for `vpsignd'
-.*:191: Error: extended GPR cannot be used as base/index for `vpsignw'
-.*:192: Error: extended GPR cannot be used as base/index for `vpsignw'
-.*:193: Error: extended GPR cannot be used as base/index for `vptest'
-.*:194: Error: extended GPR cannot be used as base/index for `vptest'
-.*:195: Error: extended GPR cannot be used as base/index for `vrcpps'
-.*:196: Error: extended GPR cannot be used as base/index for `vrcpps'
-.*:197: Error: extended GPR cannot be used as base/index for `vrcpss'
+.*:112: Error: no EVEX encoding for `vaesimc'
+.*:113: Error: no EVEX encoding for `vaeskeygenassist'
+.*:114: Error: no EVEX encoding for `vblendpd'
+.*:115: Error: no EVEX encoding for `vblendpd'
+.*:116: Error: no EVEX encoding for `vblendps'
+.*:117: Error: no EVEX encoding for `vblendps'
+.*:118: Error: no EVEX encoding for `vblendvpd'
+.*:119: Error: no EVEX encoding for `vblendvpd'
+.*:120: Error: no EVEX encoding for `vblendvps'
+.*:121: Error: no EVEX encoding for `vblendvps'
+.*:122: Error: no EVEX encoding for `vdppd'
+.*:123: Error: no EVEX encoding for `vdpps'
+.*:124: Error: no EVEX encoding for `vdpps'
+.*:125: Error: no EVEX encoding for `vhaddpd'
+.*:126: Error: no EVEX encoding for `vhaddpd'
+.*:127: Error: no EVEX encoding for `vhsubps'
+.*:128: Error: no EVEX encoding for `vhsubps'
+.*:129: Error: no EVEX encoding for `vlddqu'
+.*:130: Error: no EVEX encoding for `vlddqu'
+.*:131: Error: no EVEX encoding for `vldmxcsr'
+.*:132: Error: no EVEX encoding for `vmaskmovpd'
+.*:133: Error: no EVEX encoding for `vmaskmovpd'
+.*:134: Error: no EVEX encoding for `vmaskmovpd'
+.*:135: Error: no EVEX encoding for `vmaskmovpd'
+.*:136: Error: no EVEX encoding for `vmaskmovps'
+.*:137: Error: no EVEX encoding for `vmaskmovps'
+.*:138: Error: no EVEX encoding for `vmaskmovps'
+.*:139: Error: no EVEX encoding for `vmaskmovps'
+.*:140: Error: no EVEX encoding for `vmovmskpd'
+.*:141: Error: no EVEX encoding for `vmovmskpd'
+.*:142: Error: no EVEX encoding for `vmovmskps'
+.*:143: Error: no EVEX encoding for `vmovmskps'
+.*:144: Error: no EVEX encoding for `vpblendd'
+.*:145: Error: no EVEX encoding for `vpblendd'
+.*:146: Error: no EVEX encoding for `vpblendvb'
+.*:147: Error: no EVEX encoding for `vpblendvb'
+.*:148: Error: no EVEX encoding for `vpblendw'
+.*:149: Error: no EVEX encoding for `vpblendw'
+.*:150: Error: no EVEX encoding for `vpcmpeqb'
+.*:151: Error: no EVEX encoding for `vpcmpeqd'
+.*:152: Error: no EVEX encoding for `vpcmpeqq'
+.*:153: Error: no EVEX encoding for `vpcmpeqw'
+.*:154: Error: no EVEX encoding for `vpcmpestri'
+.*:155: Error: no EVEX encoding for `vpcmpestrm'
+.*:156: Error: no EVEX encoding for `vpcmpgtb'
+.*:157: Error: no EVEX encoding for `vpcmpgtd'
+.*:158: Error: no EVEX encoding for `vpcmpgtq'
+.*:159: Error: no EVEX encoding for `vpcmpgtw'
+.*:160: Error: no EVEX encoding for `vpcmpistri'
+.*:161: Error: no EVEX encoding for `vpcmpistrm'
+.*:162: Error: no EVEX encoding for `vperm2f128'
+.*:163: Error: no EVEX encoding for `vperm2i128'
+.*:164: Error: no EVEX encoding for `vphaddd'
+.*:165: Error: no EVEX encoding for `vphaddd'
+.*:166: Error: no EVEX encoding for `vphaddsw'
+.*:167: Error: no EVEX encoding for `vphaddsw'
+.*:168: Error: no EVEX encoding for `vphaddw'
+.*:169: Error: no EVEX encoding for `vphaddw'
+.*:170: Error: no EVEX encoding for `vphminposuw'
+.*:171: Error: no EVEX encoding for `vphsubd'
+.*:172: Error: no EVEX encoding for `vphsubd'
+.*:173: Error: no EVEX encoding for `vphsubsw'
+.*:174: Error: no EVEX encoding for `vphsubsw'
+.*:175: Error: no EVEX encoding for `vphsubw'
+.*:176: Error: no EVEX encoding for `vphsubw'
+.*:177: Error: no EVEX encoding for `vpmaskmovd'
+.*:178: Error: no EVEX encoding for `vpmaskmovd'
+.*:179: Error: no EVEX encoding for `vpmaskmovd'
+.*:180: Error: no EVEX encoding for `vpmaskmovd'
+.*:181: Error: no EVEX encoding for `vpmaskmovq'
+.*:182: Error: no EVEX encoding for `vpmaskmovq'
+.*:183: Error: no EVEX encoding for `vpmaskmovq'
+.*:184: Error: no EVEX encoding for `vpmaskmovq'
+.*:185: Error: no EVEX encoding for `vpmovmskb'
+.*:186: Error: no EVEX encoding for `vpmovmskb'
+.*:187: Error: no EVEX encoding for `vpsignb'
+.*:188: Error: no EVEX encoding for `vpsignb'
+.*:189: Error: no EVEX encoding for `vpsignd'
+.*:190: Error: no EVEX encoding for `vpsignd'
+.*:191: Error: no EVEX encoding for `vpsignw'
+.*:192: Error: no EVEX encoding for `vpsignw'
+.*:193: Error: no EVEX encoding for `vptest'
+.*:194: Error: no EVEX encoding for `vptest'
+.*:195: Error: no EVEX encoding for `vrcpps'
+.*:196: Error: no EVEX encoding for `vrcpps'
+.*:197: Error: no EVEX encoding for `vrcpss'
 .*:198: Error: .* 4 bits for `vroundpd'
 .*:199: Error: .* 4 bits for `vroundps'
 .*:200: Error: .* 4 bits for `vroundsd'
 .*:201: Error: .* 4 bits for `vroundss'
-.*:202: Error: extended GPR cannot be used as base/index for `vrsqrtps'
-.*:203: Error: extended GPR cannot be used as base/index for `vrsqrtps'
-.*:204: Error: extended GPR cannot be used as base/index for `vrsqrtss'
-.*:205: Error: extended GPR cannot be used as base/index for `vstmxcsr'
-.*:206: Error: extended GPR cannot be used as base/index for `vtestpd'
-.*:207: Error: extended GPR cannot be used as base/index for `vtestpd'
-.*:208: Error: extended GPR cannot be used as base/index for `vtestps'
-.*:209: Error: extended GPR cannot be used as base/index for `vtestps'
+.*:202: Error: no EVEX encoding for `vrsqrtps'
+.*:203: Error: no EVEX encoding for `vrsqrtps'
+.*:204: Error: no EVEX encoding for `vrsqrtss'
+.*:205: Error: no EVEX encoding for `vstmxcsr'
+.*:206: Error: no EVEX encoding for `vtestpd'
+.*:207: Error: no EVEX encoding for `vtestpd'
+.*:208: Error: no EVEX encoding for `vtestps'
+.*:209: Error: no EVEX encoding for `vtestps'
+.*: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
+	{vex} andn %r16, %r16, %r16