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

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

> On 05.12.2023 14:31, Cui, Lili wrote:
> >> On 24.11.2023 08:02, Cui, Lili wrote:
> >>> @@ -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;
> >             }
> 
> Oh, I see. That's pretty remote. How about replacing "the same way"?
> Perhaps
> "Rex2 reuses i.vex as they both encode i.tm.opcode_space in their prefixes"?
> 

Done.

> While in that form it's fine to remain in a code comment, just a general
> clarification: When I say something wants saying in the "description", it's
> (almost) always that I mean the patch description, not anything else.
> 

I see.

> >> 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.  */
> 
> Nit: Missing blank.
> 

Done.

> >             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;
> >         }
> 
> I won't insist on further changes, but imo as you're adding comments, also
> adding a comment to this last if() (which finally takes care of the 8-bit reg
> special case) would be advisable.
> 

Added.

          /* In 64-bit mode, the following byte registers cannot be accessed
             if using the Rex and Rex2 prefix: AH, BH, CH, DH */
          if (!(i.op[1].regs->reg_flags & (RegRex | RegRex2)) && base_regnum < 4)
            j += 8;

> >>> +/* 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.
> 
> Changing InOutPortReg of course isn't going to be easy. But that also wasn't
> what we had discussed. Instead (I thought) we agreed on ...
> 
> > @@ -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;
> 
> amending this code to also correctly set i.op[].regs. Perhaps it would also be
> best to actually clear i.base_reg (for there not being any memory operand).
> (FTAOD: All of this in a separate prereq patch, not here. The code creating
> inconsistent state has been a [latent] bug for a long time.)
> 

Added i.base_reg = NULL. Just discussing it here, I'll create a new patch for it.

@@ -13016,6 +13016,8 @@ 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.base_reg = NULL;
          i.input_output_operand = true;
          return 1;
        }

> >>> --- 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.
> 
> Except there's no "rex2" prefix according to the present implementation.
>
 
Remove them for current implementation.

@item
@samp{@{rex2@}} -- prefer REX2 prefix for integer and legacy vector
instructions (APX_F only).

> >>> --- 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.
> 
> Assuming (again) you mean 0x0f 0x8*, how did you not find it? Or wait,
> depends on what "opcode table" here means: The manual's or opcodes/i386-
> opc.tbl? The latter of course doesn't have them, as they're ...
> 
> >>> +	{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
> 
> ... the disp32/disp16 forms of these branches, which are created only during
> relaxation.
>

Oh,  I see,  I found them in sdm and added testcase for them.

        #All opcodes in the row 0x8* (map1) prefixed REX2 are illegal.
        {rex2} jo     .+6+0x90909090
        {rex2} jno    .+6+0x90909090
        {rex2} jb     .+6+0x90909090
        {rex2} jae    .+6+0x90909090
        {rex2} je     .+6+0x90909090
        {rex2} jne    .+6+0x90909090
        {rex2} jbe    .+6+0x90909090
        {rex2} ja     .+6+0x90909090
        {rex2} js     .+6+0x90909090
        {rex2} jns    .+6+0x90909090
        {rex2} jp     .+6+0x90909090
        {rex2} jnp    .+6+0x90909090
        {rex2} jl     .+6+0x90909090
        {rex2} jge    .+6+0x90909090
        {rex2} jle    .+6+0x90909090
        {rex2} jg     .+6+0x90909090
 
> >>> +  /* 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;
> 
> Can length be 0? I didn't think so, and then
> 
>    base_opcode = opcode >> (8 * length - 8);
> 
> would be all you need.
>

yes good way.

> Also in the comment, I think it would be slightly better to say "ModR/M-like
> byte".
> 

Done.

Thanks,
Lili.