[PATCH V2 3/8] Support APX GPR32 with extend evex prefix

[Cui, Lili]

> Subject: Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix
> 
> On 03.11.2023 17:50, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -3672,9 +3672,10 @@ install_template (const insn_template *t)
> >    /* Dual VEX/EVEX templates need stripping one of the possible variants.
> */
> >    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >    {
> > -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> > -	   || maybe_cpu (t, CpuFMA))
> > -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
> > +    if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> > +	 || maybe_cpu (t, CpuFMA) ||  maybe_cpu (t, CpuAMX_TILE))
> > +	&& (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)
> > +	    || maybe_cpu (t, CpuAPX_F)))
> 
> Something's odd with formatting (indentation) here. I first thought I had
> screwed up in my patch, but looking back things appear to be correct there.
> 

Rebase  with master. 

> > @@ -3689,12 +3690,11 @@ install_template (const insn_template *t)
> >  		i.tm.cpu.bitfield.cpuavx = 1;
> >  	      else
> >  		{
> > -		  gas_assert (!i.tm.cpu.bitfield.isa);
> >  		  i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
> 
> You may not remove the assertion here, or else the assignment could silently
> overwrite a value. Can you explain why you did the removal?
>

Rebased with master. Sorry I just saw the comments here.

> > @@ -3885,6 +3885,14 @@ is_any_vex_encoding (const insn_template *t)
> >    return t->opcode_modifier.vex || is_evex_encoding (t);  }
> >
> > +static INLINE bool
> > +is_any_apx_evex_encoding (void)
> > +{
> > +  return i.rex2 || i.tm.opcode_space == SPACE_EVEXMAP4
> > +    || (i.vex.register_specifier
> > +	&& i.vex.register_specifier->reg_flags & RegRex2); }
> 
> The use of i.rex2 here doesn't fit the name; the sole user has first checked
> that no legacy encoding is going to be used, and that's a prereq here. Such a
> prereq needs spelling out, such that one can be easily aware when possibly
> adding another caller.
> 
> Also, what does "any" stand for in the name here ...
> 

How about "check_if_any_vex_is_evex_apx_encoding ()" ?

> >  static INLINE bool
> >  is_any_apx_rex2_encoding (void)
> 
> ... and also the one here? In is_any_vex_encoding() it refers to VEX/XOP/EVEX.
> 

Changed it to  is_apx_rex2_encoding.

> > @@ -4161,6 +4169,27 @@ build_rex2_prefix (void)
> >  		    | (i.rex2 << 4) | i.rex);
> >  }
> >
> > +/* Build the EVEX prefix (4-byte) for evex insn
> > +   | 62h |
> > +   | `R`X`B`R' | B'mmm |
> > +   | W | v`v`v`v | `x' | pp |
> > +   | z| L'L | b | `v | aaa |
> > +*/
> > +static void
> > +build_apx_evex_prefix (void)
> > +{
> > +  build_evex_prefix ();
> > +  if (i.rex2 & REX_R)
> > +    i.vex.bytes[1] &= 0xef;
> 
> I think this would read easier as ~0x10 (similarly below then). I also think ...
> 

Done.

> > +  if (i.vex.register_specifier
> > +      && register_number (i.vex.register_specifier) > 0xf)
> > +    i.vex.bytes[3] &= 0xf7;
> > +  if (i.rex2 & REX_B)
> > +    i.vex.bytes[1] |= 0x08;
> 
> ... it would help if the byte 1 updates were kept together (the compiler may
> even produce better code then), and maybe ...
> 
> > +  if (i.rex2 & REX_X)
> > +    i.vex.bytes[2] &= 0xfb;
> 
> ... the byte 2 update ahead of the byte 3 one.
> 

Done.

> Also why do you use register_number() here, and not the RegRex2 flag?
> 

RegRex2 flag is better, changed it.

> > @@ -5624,19 +5653,42 @@ md_assemble (char *line)
> >  	}
> >
> >        /* Check for explicit REX2 prefix.  */
> > -      if (i.rex2 || i.rex2_encoding)
> > +      if (i.rex2_encoding)
> 
> This change ...
> 
> >  	{
> >  	  as_bad (_("REX2 prefix invalid with `%s'"), insn_name (&i.tm));
> 
> ... further invalidates this message. Yet iirc you said you removed the
> i.rex2 part of the check anyway from patch 1.
> 
> >  	  return;
> >  	}
> >
> > -      if (i.tm.opcode_modifier.vex)
> > +      if (is_any_apx_evex_encoding ())
> > +	{
> > +	  if (i.tm.opcode_space == SPACE_EVEXMAP4 &&
> (i.prefix[DATA_PREFIX] != 0))
> > +	    {
> > +	      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
> 
> Perhaps better assert that no other embedded prefix was already recorded
> here?

Added the code as below, I added as_bad instead of assert, I think this is a input error and not a gas internal error, right?  Besides REX_PREFIX?

      if (check_if_any_vex_is_evex_apx_encoding ())
        {
          if (i.tm.opcode_space == SPACE_EVEXMAP4 && (i.prefix[DATA_PREFIX] != 0))
            {

              i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
              i.prefix[DATA_PREFIX] = 0;

              /*  Prefixes other than the rex prefix cannot be used with the data prefix.  */
              const unsigned char *p = i.prefix;

              for (j = 0; j < ARRAY_SIZE (i.prefix); ++j, ++p)
                {
                  if (!*p)
                    continue;

                  switch (j)
                    {
                    case DATA_PREFIX:
                    case REX_PREFIX:
                      break;
                    default:
                      as_bad (_("unexpecting prefix %x together with DATA "
                                "prefix in front of evex-promoted apx "
                                "instruction "), *p);
                      return;
                    }
                }
            }

          build_apx_evex_prefix ();
      }

> 
> > +	      i.prefix[DATA_PREFIX] = 0;
> > +	    }
> > +
> > +	  build_apx_evex_prefix ();
> > +
> > +	  /* Encode the NDD bit of the instruction promoted from the legacy
> > +	     space.  */
> > +	  if (i.vex.register_specifier && i.tm.opcode_space ==
> SPACE_EVEXMAP4)
> > +	    i.vex.bytes[3] |= 0x10;
> 
> Why the restriction to map 3? And why is this not part of
> build_apx_evex_prefix()?

It is the ND bit, which is added in advance here. After reading the comments below, I'll move this to an NDD patch.

> 
> > +	  /* Encode the NF bit of the instruction promoted from legacy and
> vex
> > +	     space.  */
> > +	  if (i.has_nf)
> > +	    i.vex.bytes[3] |= 0x04;
> 
> This wants to move to the patch actually introducing NF handling.
> 

Done.

> > @@ -5663,16 +5715,17 @@ md_assemble (char *line)
> >       instruction already has a prefix, we need to convert old
> >       registers to new ones.  */
> >
> > -  if ((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> > -       && (i.op[0].regs->reg_flags & RegRex64) != 0)
> > -      || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> > -	  && (i.op[1].regs->reg_flags & RegRex64) != 0)
> > -      || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> > -	   || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > -	  && (i.rex != 0 || i.rex2 != 0)))
> > +  if (!is_any_vex_encoding (&i.tm)
> > +      && ((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> > +	   && (i.op[0].regs->reg_flags & RegRex64) != 0)
> > +	  || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> > +	      && (i.op[1].regs->reg_flags & RegRex64) != 0)
> > +	  || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> > +	       || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > +	      && (i.rex != 0 || i.rex2 != 0))))
> 
> I'm a little puzzled by this (hard to read) adjustment:
> is_any_vex_encoding() basically excludes most new templates added here,
> yet for those permitting Reg8 you still need to enforce the EVEX-imposed
> restriction.
> 

Moved is_any_vex_encoding() into if body to constrain i.rex |= REX_OPCODE.

if (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
        && (i.op[0].regs->reg_flags & RegRex64) != 0)
       || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
           && (i.op[1].regs->reg_flags & RegRex64) != 0)
       || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
            || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
           && (i.rex != 0 || i.rex2 != 0))))
    {
      int x;

      if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
        i.rex |= REX_OPCODE;

> > @@ -7043,7 +7096,7 @@ VEX_check_encoding (const insn_template *t)
> > static int  check_EgprOperands (const insn_template *t)  {
> > -  if (t->opcode_modifier.noegpr)
> > +  if (t->opcode_modifier.noegpr && !need_evex_encoding())
> >      {
> >        for (unsigned int op = 0; op < i.operands; op++)
> >  	{
> 
> What is this change about?
> 

After merging vex and evex, evex exits here early and all evex supports egpr.

> > @@ -14252,6 +14306,9 @@ static bool check_register (const reg_entry
> > *r)
> >
> >    if (r->reg_flags & RegRex2)
> >      {
> > +      if (is_evex_encoding (current_templates->start))
> > +	i.vec_encoding = vex_encoding_evex;
> 
> What if the APX template isn't first in the group?
> 

If apx_f is not supported, it will return false, just after this code. Oh, better to move it to the back. Done.

  if (r->reg_flags & RegRex2)
    {
      if (current_templates->start->opcode_modifier.evex)
        i.vec_encoding = vex_encoding_evex;

      if (!cpu_arch_flags.bitfield.cpuapx_f
          || flag_code != CODE_64BIT)
        return false;
    }

> > --- a/opcodes/i386-opc.h
> > +++ b/opcodes/i386-opc.h
> > @@ -975,6 +975,7 @@ typedef struct insn_template
> >       1: 0F opcode prefix / space.
> >       2: 0F38 opcode prefix / space.
> >       3: 0F3A opcode prefix / space.
> > +     4: EVEXMAP4 opcode prefix / space.
> >       5: EVEXMAP5 opcode prefix / space.
> >       6: EVEXMAP6 opcode prefix / space.
> >       7: VEXMAP7 opcode prefix / space.
> > @@ -986,6 +987,7 @@ typedef struct insn_template
> >  #define SPACE_0F	1
> >  #define SPACE_0F38	2
> >  #define SPACE_0F3A	3
> > +#define SPACE_EVEXMAP4  4
> >  #define SPACE_EVEXMAP5	5
> >  #define SPACE_EVEXMAP6	6
> >  #define SPACE_VEXMAP7	7
> 
> Nit: Please can padding here match surrounding code?
> 

Done.

> > @@ -338,6 +342,7 @@ adc, 0x14, 0, W|No_sSuf, {
> > Imm8|Imm16|Imm32|Imm32S, Acc|Byte|Word|Dword|Qword }  adc,
> 0x80/2, 0,
> > W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S,
> >
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> x }
> >
> >  neg, 0xf6/3, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> x }
> > +
> >  not, 0xf6/2, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> x }
> >
> >  aaa, 0x37, No64, NoSuf, {}
> 
> Nit: In an already overlarge patch it is extremely helpful if any unrelated
> changed could be omitted.
> 

Done.

> > @@ -1316,13 +1321,16 @@ getsec, 0xf37, SMX, NoSuf, {}
> >
> >  invept, 0x660f3880, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> > Oword|Unspecified|BaseIndex, Reg32 }  invept, 0x660f3880, EPT&x64,
> > Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> > +invept, 0xf3f0, APX_F_64&EPT, Modrm|NoSuf|EVex128|EVexMap4, {
> > +Oword|Unspecified|BaseIndex, Reg64 }
> >  invvpid, 0x660f3881, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> > Oword|Unspecified|BaseIndex, Reg32 }  invvpid, 0x660f3881, EPT&x64,
> > Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> > +invvpid, 0xf3f1, APX_F_64&EPT, Modrm|NoSuf|EVex128|EVexMap4, {
> > +Oword|Unspecified|BaseIndex, Reg64 }
> >
> >  // INVPCID instruction
> >
> >  invpcid, 0x660f3882, INVPCID&No64, Modrm|IgnoreSize|NoSuf, {
> > Oword|Unspecified|BaseIndex, Reg32 }  invpcid, 0x660f3882,
> > INVPCID&x64, Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex,
> Reg64
> > }
> > +invpcid, 0xf3f2, APX_F_64&INVPCID, Modrm|NoSuf|EVex128|EVexMap4, {
> > +Oword|Unspecified|BaseIndex, Reg64 }
> 
> While ordering doesn't matter functionality wise, overall readability imo
> would end up improved if you had EPT&APX_F and INVPCID&APX_f here (and
> the likewise elsewhere, albeit it looks in other cases you already have things
> the other way round).
> 

Done,  it should be consistent.

> > @@ -3310,6 +3364,7 @@ prefetchit1, 0xf18/6, PREFETCHI&x64,
> > Modrm|Anysize|IgnoreSize|NoSuf, { BaseIndex  // CMPCCXADD
> instructions.
> >
> >  cmp<cc>xadd, 0x66e<cc:opc>, CMPCCXADD&x64,
> > Modrm|Vex|Space0F38|VexVVVV|SwapSources|CheckOperandSize|NoSuf,
> {
> > Reg32|Reg64, Reg32|Reg64, Dword|Qword|Unspecified|BaseIndex }
> > +cmp<cc>xadd, 0x66e<cc:opc>, CMPCCXADD&x64&APX_F_64,
> >
> +Modrm|EVex128|Space0F38|VexVVVV|SwapSources|CheckOperandSize|N
> oSuf, {
> > +Reg32|Reg64, Reg32|Reg64, Dword|Qword|Unspecified|BaseIndex }
> 
> Nit: Redundant x64.
> 

Done, thanks!

Lili.