RE: [PATCH v3 2/9] Support APX GPR32 with rex2 prefix

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

> On 24.11.2023 08:02, Cui, Lili wrote:
> > @@ -3865,6 +3873,12 @@ is_any_vex_encoding (const insn_template *t)
> >    return t->opcode_modifier.vex || t->opcode_modifier.evex;  }
> >
> > +static INLINE bool
> > +is_apx_rex2_encoding (void)
> > +{
> > +  return i.rex2 || i.rex2_encoding;
> > +}
> 
> This function is used just once. Do we really need it? Or else why don't you
> use it near the end of md_assemble()?
> 

Yes, I also found this issue and used this function instead of " (i.rex2 != 0 || i.rex2_encoding)" at the end of md_assemble().

> > @@ -4120,6 +4134,21 @@ build_evex_prefix (void)
> >      i.vex.bytes[3] |= i.mask.reg->reg_num;  }
> >
> > +/* Build (2 bytes) rex2 prefix.
> > +   | D5h |
> > +   | m | R4 X4 B4 | W R X B |
> > +*/
> > +static void
> > +build_rex2_prefix (void)
> > +{
> > +  /* Rex2 reuses i.vex because they handle i.tm.opcode_space the
> > +same.  */
> 
> How do they handle it the same? (Also I don't think this is useful as a code
> comment; it instead belongs in the description imo.)
> 

Moved the comment to the functions description.

/* Build (2 bytes) rex2 prefix.
   | D5h |
   | m | R4 X4 B4 | W R X B |

   Rex2 reuses i.vex as they handle i.tm.opcode_space the same way.  */
static void
build_rex2_prefix (void)


In function "output_insn",  some handle like this.

      if (!i.vex.length)
        switch (i.tm.opcode_space)
          {
          case SPACE_BASE:
            break;
          case SPACE_0F:
            ++j;
            break;
          case SPACE_0F38:
          case SPACE_0F3A:
            j += 2;
            break;
          default:
            abort ();
          }
.....
         if (!i.vex.length
              && i.tm.opcode_space != SPACE_BASE)
            {
              *p++ = 0x0f;
              if (i.tm.opcode_space != SPACE_0F)
                *p++ = i.tm.opcode_space == SPACE_0F38
                       ? 0x38 : 0x3a;
            }

> > +  i.vex.length = 2;
> > +  i.vex.bytes[0] = 0xd5;
> > +  /* For the W R X B bits, the variables of rex prefix will be
> > +reused.  */
> > +  i.vex.bytes[1] = ((i.tm.opcode_space << 7)
> > +		    | (i.rex2 << 4) | i.rex);
> > +}
> > +
> >  static void
> >  process_immext (void)
> >  {
> > @@ -4385,12 +4414,16 @@ optimize_encoding (void)
> >  	  i.suffix = 0;
> >  	  /* Convert to byte registers.  */
> >  	  if (i.types[1].bitfield.word)
> > -	    j = 16;
> > -	  else if (i.types[1].bitfield.dword)
> > +	    /* There are 40 8-bit registers.  */
> >  	    j = 32;
> > +	  else if (i.types[1].bitfield.dword)
> > +	    /* 32 8-bit registers + 32 16-bit registers.  */
> > +	    j = 64;
> >  	  else
> > -	    j = 48;
> > -	  if (!(i.op[1].regs->reg_flags & RegRex) && base_regnum < 4)
> > +	    /* 32 8-bit registers + 32 16-bit registers
> > +	       + 32 32-bit registers.  */
> > +	    j = 96;
> > +	  if (!(i.op[1].regs->reg_flags & (RegRex | RegRex2)) && base_regnum
> > +< 4)
> >  	    j += 8;
> >  	  i.op[1].regs -= j;
> >  	}
> 
> I did comment on, in particular, the 8-bit register counts before.
> Afaict the comments above are nevertheless unchanged and hence still not
> really correct.
> 

Changed to :

      if (flag_code == CODE_64BIT || base_regnum < 4)
        {
          i.types[1].bitfield.byte = 1;
          /* Ignore the suffix.  */
          i.suffix = 0;
          /* Convert to byte registers. 8-bit registers are special,
             RegRex64 and non-RegRex64 each have 8 registers.  */
          if (i.types[1].bitfield.word)
            /* 32 (or 40) 8-bit registers.  */
            j = 32;
          else if (i.types[1].bitfield.dword)
            /* 32 (or 40)8-bit registers + 32 16-bit registers.  */
            j = 64;
          else
            /* 32 (or 40) 8-bit registers + 32 16-bit registers
               + 32 32-bit registers.  */
            j = 96;

          if (!(i.op[1].regs->reg_flags & (RegRex | RegRex2)) && base_regnum < 4)
            j += 8;
          i.op[1].regs -= j;
        }

> > @@ -5576,6 +5615,13 @@ md_assemble (char *line)
> >  	  return;
> >  	}
> >
> > +      /* Check for explicit REX2 prefix.  */
> > +      if (i.rex2_encoding)
> > +	{
> > +	  as_bad (_("REX2 prefix invalid with `%s'"), insn_name (&i.tm));
> > +	  return;
> > +	}
> 
> Again I'm pretty sure I pointed out before that i.rex2_encoding reflects use of
> {rex2}. Which then the error message should correctly refer to.
> 

Changed to 

      /* Check for explicit REX2 prefix.  */
      if (i.rex2_encoding)
        {
          as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
          return;
        }

> > @@ -5615,11 +5661,12 @@ md_assemble (char *line)
> >  	  && (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.rex != 0 || i.rex2 != 0)))
> >      {
> >        int x;
> >
> > -      i.rex |= REX_OPCODE;
> > +      if (!i.rex2)
> > +	i.rex |= REX_OPCODE;
> >        for (x = 0; x < 2; x++)
> >  	{
> >  	  /* Look for 8 bit operand that uses old registers.  */ @@ -5630,7
> > +5677,7 @@ md_assemble (char *line)
> >  	      /* In case it is "hi" register, give up.  */
> >  	      if (i.op[x].regs->reg_num > 3)
> >  		as_bad (_("can't encode register '%s%s' in an "
> > -			  "instruction requiring REX prefix."),
> > +			  "instruction requiring REX/REX2 prefix."),
> >  			register_prefix, i.op[x].regs->reg_name);
> >
> >  	      /* Otherwise it is equivalent to the extended register.
> > @@ -5642,11 +5689,11 @@ md_assemble (char *line)
> >  	}
> >      }
> >
> > -  if (i.rex == 0 && i.rex_encoding)
> > +  if ((i.rex == 0 && i.rex_encoding) || (i.rex2 == 0 &&
> > + i.rex2_encoding))
> 
> Doesn't this want to be
> 
>   if (i.rex == 0 && i.rex2 == 0 && (i.rex_encoding || i.rex2_encoding))
> 
> ?

Done.

> 
> >      {
> >        /* Check if we can add a REX_OPCODE byte.  Look for 8 bit operand
> >  	 that uses legacy register.  If it is "hi" register, don't add
> > -	 the REX_OPCODE byte.  */
> > +	 rex and rex2 prefix.  */
> >        int x;
> >        for (x = 0; x < 2; x++)
> >  	if (i.types[x].bitfield.class == Reg @@ -5656,6 +5703,7 @@
> > md_assemble (char *line)
> >  	  {
> >  	    gas_assert (!(i.op[x].regs->reg_flags & RegRex));
> >  	    i.rex_encoding = false;
> > +	    i.rex2_encoding = false;
> >  	    break;
> >  	  }
> >
> > @@ -5663,7 +5711,13 @@ md_assemble (char *line)
> >  	i.rex = REX_OPCODE;
> >      }
> >
> > -  if (i.rex != 0)
> > +  if (i.rex2 != 0 || i.rex2_encoding)
> > +    {
> > +      build_rex2_prefix ();
> > +      /* The individual REX.RXBW bits got consumed.  */
> > +      i.rex &= REX_OPCODE;
> > +    }
> > +  else if (i.rex != 0)
> >      add_prefix (REX_OPCODE | i.rex);
> >
> >    insert_lfence_before ();
> > @@ -5834,6 +5888,10 @@ parse_insn (const char *line, char *mnemonic,
> bool prefix_only)
> >  		  /* {rex} */
> >  		  i.rex_encoding = true;
> >  		  break;
> > +		case Prefix_REX2:
> > +		  /* {rex2} */
> > +		  i.rex2_encoding = true;
> > +		  break;
> >  		case Prefix_NoOptimize:
> >  		  /* {nooptimize} */
> >  		  i.no_optimize = true;
> > @@ -6971,6 +7029,45 @@ VEX_check_encoding (const insn_template *t)
> >    return 0;
> >  }
> >
> > +/* Check if Egprs operands are valid for the instruction.  */
> > +
> > +static int
> > +check_EgprOperands (const insn_template *t) {
> > +  if (!t->opcode_modifier.noegpr)
> > +    return 0;
> > +
> > +  for (unsigned int op = 0; op < i.operands; op++)
> > +    {
> > +      if (i.types[op].bitfield.class != Reg
> > +	  /* Special case for (%dx) while doing input/output op */
> > +	  || i.input_output_operand)
> 
> Didn't we agree that this extra condition isn't necessary, once the producer
> site correctly updates all state (which was supposed to be done in a small
> prereq patch)?
> 

I tried adding "Unspecified | BaseIndex" to the InOutPortReg, then some related instructions had two memory operands, so it raised a lot of invalid test case fail, and more ugly code needed to be added. In the end, I felt that this simple modification might be better.

@@ -13137,6 +13137,7 @@ i386_att_operand (char *operand_string)
          && !operand_type_check (i.types[this_operand], disp))
        {
          i.types[this_operand] = i.base_reg->reg_type;
+         i.types[this_operand].bitfield.class = 0;
          i.input_output_operand = true;
          return 1;

> > @@ -7107,7 +7204,9 @@ match_template (char mnem_suffix)
> >        /* Do not verify operands when there are none.  */
> >        if (!t->operands)
> >  	{
> > -	  if (VEX_check_encoding (t))
> > +	  /* When there are no operands, we still need to use the
> > +	     check_EgprOperands function to check whether {rex2} is valid.  */
> > +	  if (VEX_check_encoding (t) || check_EgprOperands (t))
> 
> As before imo either the function name wants changing (so it becomes
> reasonable to use here, without the need for a comment explaining the
> oddity), or you simply open-code the sole check that is needed here
> (afaict: t->opcode_modifier.noegpr && i.rex2_encoding).
> 

Changed to 

          if (VEX_check_encoding (t))
            {
              specific_error = progress (i.error);
              continue;
            }

          /* Check if pseudo prefix {rex2} is valid.  */
          if (t->opcode_modifier.noegpr && i.rex2_encoding)
            {
              i.error = invalid_pseudo_prefix;
              specific_error = progress (i.error);
              continue;
            }

> > @@ -7443,6 +7542,13 @@ match_template (char mnem_suffix)
> >  	  continue;
> >  	}
> >
> > +      /* Check if EGRPS operands(r16-r31) are valid.  */
> 
> EGPR?
> 

Done.

> > --- a/gas/doc/c-i386.texi
> > +++ b/gas/doc/c-i386.texi
> > @@ -217,6 +217,7 @@ accept various extension mnemonics.  For example,
> > @code{avx10.1/256},  @code{avx10.1/128},  @code{user_msr},
> > +@code{apx_f},
> >  @code{amx_int8},
> >  @code{amx_bf16},
> >  @code{amx_fp16},
> > @@ -983,6 +984,9 @@ Different encoding options can be specified via
> pseudo prefixes:
> >  instructions (x86-64 only).  Note that this differs from the
> > @samp{rex}  prefix which generates REX prefix unconditionally.
> >
> > +@item
> > +@samp{@{rex2@}} -- encode with REX2 prefix
> 
> This isn't in line with what's said for {rex}. Iirc we were in agreement that we
> want both to behave consistently. In which case documentation also needs to
> describe them consistently.
> 

Changed to 

@item
@samp{@{rex2@}} -- prefer REX2 prefix for integer and legacy vector
instructions (APX_F only).  Note that this differs from the @samp{rex2}
prefix which generates REX2 prefix unconditionally.

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-rex2.s
> > @@ -0,0 +1,86 @@
> > +# Check 64bit instructions with rex2 prefix encoding
> > +         leal	(,%r16), %eax
> > +         leal	(,%r17), %eax
> > +         leal	(,%r18), %eax
> > +         leal	(,%r19), %eax
> > +         leal	(,%r20), %eax
> > +         leal	(,%r21), %eax
> > +         leal	(,%r22), %eax
> > +         leal	(,%r23), %eax
> > +         leal	(,%r24), %eax
> > +         leal	(,%r25), %eax
> > +         leal	(,%r26), %eax
> > +         leal	(,%r27), %eax
> > +         leal	(,%r28), %eax
> > +         leal	(,%r29), %eax
> > +         leal	(,%r30), %eax
> > +         leal	(,%r31), %eax
> > +## REX.B4 bit
> 
> Further up you properly say REX2. Here and below it's only REX?
> 

Done.

> > --- a/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > +++ b/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > @@ -5,3 +5,61 @@ pseudos:
> >  	{rex} vmovaps %xmm7,%xmm2
> >  	{rex} vmovaps %xmm17,%xmm2
> >  	{rex} rorx $7,%eax,%ebx
> > +	{rex2} vmovaps %xmm7,%xmm2
> > +	{rex2} xsave (%rax)
> > +	{rex2} xsaves (%ecx)
> > +	{rex2} xsaves64 (%ecx)
> > +	{rex2} xsavec (%ecx)
> > +	{rex2} xrstors (%ecx)
> > +	{rex2} xrstors64 (%ecx)
> > +
> > +	#All opcodes in the row 0xa* prefixed REX2 are illegal.
> > +	#{rex2} test (0xa8) is a special case, it will remap to test (0xf6)
> > +	{rex2} mov    0x90909090,%al
> > +	{rex2} movabs 0x1,%al
> > +	{rex2} cmpsb  %es:(%edi),%ds:(%esi)
> > +	{rex2} lodsb
> > +	{rex2} lods   %ds:(%esi),%al
> > +	{rex2} lodsb   (%esi)
> > +	{rex2} movs
> > +	{rex2} movs   (%esi), (%edi)
> > +	{rex2} scasl
> > +	{rex2} scas   %es:(%edi),%eax
> > +	{rex2} scasb   (%edi)
> > +	{rex2} stosb
> > +	{rex2} stosb   (%edi)
> > +	{rex2} stos   %eax,%es:(%edi)
> > +
> > +	#All opcodes in the row 0x7* prefixed REX2 are illegal.
> 
> This also covers map 1 row 8, doesn't it?
> 

No, I didn't find 0xf8* in opcode table.

> > +	{rex2} jo     .+2-0x70
> > +	{rex2} jno    .+2-0x70
> > +	{rex2} jb     .+2-0x70
> > +	{rex2} jae    .+2-0x70
> > +	{rex2} je     .+2-0x70
> > +	{rex2} jne    .+2-0x70
> > +	{rex2} jbe    .+2-0x70
> > +	{rex2} ja     .+2-0x70
> > +	{rex2} js     .+2-0x70
> > +	{rex2} jns    .+2-0x70
> > +	{rex2} jp     .+2-0x70
> > +	{rex2} jnp    .+2-0x70
> > +	{rex2} jl     .+2-0x70
> > +	{rex2} jge    .+2-0x70
> > +	{rex2} jle    .+2-0x70
> > +	{rex2} jg     .+2-0x70
> > +
> > +	#All opcodes in the row 0x7* prefixed REX2 are illegal.
> > +	{rex2} in $0x90,%al
> > +	{rex2} in $0x90
> > +	{rex2} out $0x90,%al
> > +	{rex2} out $0x90
> > +	{rex2} jmp  *%eax
> > +	{rex2} loop foo
> > +	#All opcodes in the row 0xf3* prefixed REX2 are illegal.
> 
> This comment continues to be confusing: 0xf3 is a REP prefix. Perhaps best to
> either say "map 1" and omit the "f" or at least write 0x0f3* or slightly better
> 0x0f 0x3*.
> 

Done.

> > --- a/gas/testsuite/gas/i386/x86-64-pseudos.s
> > +++ b/gas/testsuite/gas/i386/x86-64-pseudos.s
> > @@ -360,6 +360,19 @@ _start:
> >  	{rex} movaps (%r8),%xmm2
> >  	{rex} phaddw (%rcx),%mm0
> >  	{rex} phaddw (%r8),%mm0
> > +	{rex2} mov %al,%ah
> > +	{rex2} shl %cl, %eax
> > +	{rex2} cmp %cl, %dl
> > +	{rex2} mov $1, %bl
> > +	{rex2} movl %eax,%ebx
> > +	{rex2} movl %eax,%r14d
> > +	{rex2} movl %eax,(%r8)
> > +	{rex2} movaps %xmm7,%xmm2
> > +	{rex2} movaps %xmm7,%xmm12
> > +	{rex2} movaps (%rcx),%xmm2
> > +	{rex2} movaps (%r8),%xmm2
> > +	{rex2} pmullw %mm0,%mm6
> > +
> >
> >  	movb (%rbp),%al
> >  	{disp8} movb (%rbp),%al
> 
> No double blank lines please.
> 

Done.

> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> 
> Disassembler comments (if any) in a separate (later) mail again.
> 

OK.

> > --- a/opcodes/i386-gen.c
> > +++ b/opcodes/i386-gen.c
> > @@ -275,6 +275,8 @@ static const dependency isa_dependencies[] =
> >      "64" },
> >    { "USER_MSR",
> >      "64" },
> > +  { "APX_F",
> > +    "XSAVE|64" },
> >  };
> >
> >  /* This array is populated as process_i386_initializers() walks
> > cpu_flags[].  */ @@ -397,6 +399,7 @@ static bitfield cpu_flags[] =
> >    BITFIELD (FRED),
> >    BITFIELD (LKGS),
> >    BITFIELD (USER_MSR),
> > +  BITFIELD (APX_F),
> >    BITFIELD (MWAITX),
> >    BITFIELD (CLZERO),
> >    BITFIELD (OSPKE),
> > @@ -486,6 +489,7 @@ static bitfield opcode_modifiers[] =
> >    BITFIELD (ATTSyntax),
> >    BITFIELD (IntelSyntax),
> >    BITFIELD (ISA64),
> > +  BITFIELD (NoEgpr),
> >  };
> >
> >  #define CLASS(n) #n, n
> > @@ -1072,10 +1076,48 @@ get_element_size (char **opnd, int lineno)
> >    return elem_size;
> >  }
> >
> > +static bool
> > +rex2_disallowed (const unsigned long long opcode, unsigned int space,
> > +			       const char *cpu_flags)
> > +{
> > +  /* Prefixing XSAVE* and XRSTOR* instructions with REX2 triggers
> > +#UD.  */
> > +  if (strcmp (cpu_flags, "XSAVES") >= 0
> > +      || strcmp (cpu_flags, "XSAVEC") >= 0
> > +      || strcmp (cpu_flags, "Xsave") >= 0
> > +      || strcmp (cpu_flags, "Xsaveopt") >= 0)
> > +    return true;
> 
> Wasn't this intended to be dropped, being redundant with the opcode table
> attributes?
>

Yes, dropped.
 
> > +  /* All opcodes listed map0 0x4*, 0x7*, 0xa*, 0xe* and map1 0x3*, 0x8*
> > +     are reserved under REX2 and triggers #UD when prefixed with REX2
> > + */  if (space == 0)
> > +    switch (opcode >> 4)
> 
> Both here and ...
>
> > +      {
> > +      case 0x4:
> > +      case 0x7:
> > +      case 0xA:
> > +      case 0xE:
> > +	return true;
> > +      default:
> > +	return false;
> > +    }
> > +
> > +  if (space == SPACE_0F)
> > +    switch (opcode >> 4)
> 
> ... here, don't you also need to mask off further bits? There are quite a few
> opcodes which have a kind-of ModR/M byte encoded directly in the opcode,
> for example.
> 

Thanks for reminding. Added the code like this.

/* Some opcodes encode a ModR/M byte directly in the opcode.  */
  unsigned long long
  base_opcode = (length > 1) ? opcode >> (8 * length - 8) : opcode;

/* All opcodes listed map0 0x4*, 0x7*, 0xa*, 0xe* and map1 0x3*, 0x8*
     are reserved under REX2 and triggers #UD when prefixed with REX2 */
  if (space == 0)
    switch (base_opcode >> 4)
      {
      case 0x4:
      case 0x7:
      case 0xA:
      case 0xE:
        return true;
      default:
        return false;
    }

  if (space == SPACE_0F)
    switch (base_opcode >> 4)

> > +      {
> > +      case 0x3:
> > +      case 0x8:
> > +	return true;
> > +      default:
> > +	return false;
> > +      }
> > +
> > +  return false;
> > +}
> > +
> >  static void
> >  process_i386_opcode_modifier (FILE *table, char *mod, unsigned int
> space,
> >  			      unsigned int prefix, const char
> *extension_opcode,
> > -			      char **opnd, int lineno)
> > +			      char **opnd, int lineno, bool rex2_disallowed)
> >  {
> >    char *str, *next, *last;
> >    bitfield modifiers [ARRAY_SIZE (opcode_modifiers)]; @@ -1202,6
> > +1244,12 @@ process_i386_opcode_modifier (FILE *table, char *mod,
> unsigned int space,
> >  	  || modifiers[SAE].value))
> >      modifiers[EVex].value = EVEXDYN;
> >
> > +  /* Vex, legacy map2 and map3 and rex2_disallowed do not support
> EGPR.
> > +     For template supports both Vex and EVex allowing EGPR.  */
> 
> "Templates supporting both Vex and EVex allow EGPR."
> 

Done.

> > +  if ((modifiers[Vex].value || space > SPACE_0F || rex2_disallowed)
> > +      && !modifiers[EVex].value)
> > +    modifiers[NoEgpr].value = 1;
> > +
> >    output_opcode_modifier (table, modifiers, ARRAY_SIZE (modifiers));
> > }
> >
> > @@ -1425,8 +1473,11 @@ output_i386_opcode (FILE *table, const char
> *name, char *str,
> >  	   ident, 2 * (int)length, opcode, end, i);
> >    free (ident);
> >
> > +  /* Add some specilal handle for current entry.  */  bool
> > + has_special_handle = rex2_disallowed (opcode, space, cpu_flags);
> 
> The local variable (if one is needed in the first place) wants naming as usefully
> as the function now is named. Similarly the comment would want improving
> alonmg those lines.
> 

Dropped the local variable. Changed to

  process_i386_opcode_modifier (table, opcode_modifier, space, prefix,
                                extension_opcode, operand_types, lineno,
                                rex2_disallowed (opcode, length, space,
                                                 cpu_flags));

> >    process_i386_opcode_modifier (table, opcode_modifier, space, prefix,
> > -				extension_opcode, operand_types, lineno);
> > +				extension_opcode, operand_types, lineno,
> > +				has_special_handle);
> >
> >    process_i386_cpu_flag (table, cpu_flags, NULL, ",", "    ", lineno, CpuMax);
> >
> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> > @@ -138,6 +138,7 @@
> >  #define Vsz256 Vsz=VSZ256
> >  #define Vsz512 Vsz=VSZ512
> >
> > +
> >  // The EVEX purpose of StaticRounding appears only together with SAE.
> > Re-use  // the bit to mark commutative VEX encodings where swapping
> > the source  // operands may allow to switch from 3-byte to 2-byte VEX
> encoding.
> 
> Stray change (in general please avoid introducing double blank lines, as those
> make patch context less useful).
> 
Done.

Thanks,
Lili.