RE: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix

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

> On 24.11.2023 08:02, Cui, Lili wrote:
> > --- /dev/null
> > +++ b/opcodes/i386-dis-evex-x86-64.h
> > @@ -0,0 +1,60 @@
> > +  /* X86_64_EVEX_0F90 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F90) },  },
> > +  /* X86_64_EVEX_0F91 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F91) },  },
> > +  /* X86_64_EVEX_0F92 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F92) },  },
> > +  /* X86_64_EVEX_0F93 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F93) },  },
> > +  /* X86_64_EVEX_0F3849 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F3849_X86_64) },  },
> > +  /* X86_64_EVEX_0F384B */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F384B_X86_64) },  },
> > +  /* X86_64_EVEX_0F38F2 */
> > +  {
> > +    { Bad_Opcode },
> > +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F2) },  },
> > +  /* X86_64_EVEX_0F38F3 */
> > +  {
> > +    { Bad_Opcode },
> > +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F3) },  },
> > +  /* X86_64_EVEX_0F38F5 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F5) },  },
> > +  /* X86_64_EVEX_0F38F6 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F6) },  },
> > +  /* X86_64_EVEX_0F38F7 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F7) },  },
> > +  /* X86_64_EVEX_0F3AF0 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F3AF0) },  },
> 
> I'm puzzled here: There are two uses of EVEX_LEN_TABLE() and several more
> of VEX_LEN_TABLE(). Yet the underlying pattern of those insns is all the same. I
> may guess that this is related to PREFIX_OPCODE use in the respective VEX
> table entries, yet isn't it then cheaper overall to have VEX encodings also go
> through prefix_table[], and then sharing those entries with EVEX encodings?
> 

Done.

> What's further puzzling: When setting evex_from_vex you already check L'L ==
> 0, so there's no reason to go through evex_len_table[] / vex_len_table[].
> 

Directly use the next level of len_table[].

> > @@ -1268,7 +1296,21 @@ enum
> >    X86_64_VEX_0F38ED,
> >    X86_64_VEX_0F38EE,
> >    X86_64_VEX_0F38EF,
> > +
> >    X86_64_VEX_MAP7_F8_L_0_W_0_R_0,
> > +
> > +  X86_64_EVEX_0F90,
> > +  X86_64_EVEX_0F91,
> > +  X86_64_EVEX_0F92,
> > +  X86_64_EVEX_0F93,
> > +  X86_64_EVEX_0F3849,
> > +  X86_64_EVEX_0F384B,
> 
> For these two, won't the respective VEX enumerators and table entries do?
> 

Done.

> > @@ -4524,10 +4568,11 @@ static const struct dis386 x86_64_table[][2] =
> > {
> >
> >    /* X86_64_VEX_MAP7_F8_L_0_W_0_R_0 */
> >    {
> > -    { Bad_Opcode },
> > -    { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> > +      { Bad_Opcode },
> > +      { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> >    },
> 
> Actively corrupting indentation here?
> 

Done.

> > @@ -8733,6 +8778,17 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        dp = &prefix_table[dp->op[1].bytemode][vindex];
> >        break;
> >
> > +    case USE_X86_64_EVEX_FROM_VEX_TABLE:
> > +      ins->evex_type = evex_from_vex;
> > +      /* EVEX from evex instrucions require that EVEX.z, EVEX.L’L,
> > + EVEX.b and
> 
> "EVEX from VEX ..."?
> 

Done.

> > +	 the lower 2 bits of EVEX.aaa must be 0.  */
> > +      if ((ins->vex.mask_register_specifier & 0x3) != 0
> > +	  || ins->vex.ll != 0
> > +	  || ins->vex.zeroing != 0
> > +	  || ins->vex.b)
> > +	return &bad_opcode;
> > +
> > +      /* Fall through.  */
> >      case USE_X86_64_TABLE:
> 
> Instead of falling through here to go through x86_64_table[] (where in all
> cases the non-64-bit slot is "bad"), can't you avoid that step and go to the
> next step (uniformly the LEN one) right away, saving all those new table entries
> (along the lines of what you do below when processing into
> evex_from_legacy)?
> 

It's not very clear to me here, do you want to add the vex_len_table to delete all entries in i386-dis-evex-x86-64.h?  but in this way, there are still some instructions that need to go through x86_64_table[], such as X86_64_VEX_0F38E*.

    case USE_X86_64_EVEX_FROM_VEX_TABLE:
      ins->evex_type = evex_from_vex;
      /* EVEX from VEX instrucions require that EVEX.z, EVEX.L’L, EVEX.b and
         the lower 2 bits of EVEX.aaa must be 0.  */
      if ((ins->vex.mask_register_specifier & 0x3) != 0
          || ins->vex.ll != 0
          || ins->vex.zeroing != 0
          || ins->vex.b)
        return &bad_opcode;

     dp = &vex_len_table[dp->op[1].bytemode][0];
break;

> > @@ -8978,9 +9034,13 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        if (!fetch_code (ins->info, ins->codep + 4))
> >  	return &err_opcode;
> >        /* The first byte after 0x62.  */
> > +      if (*ins->codep & 0x8)
> > +	ins->rex2 |= REX_B;
> > +      if (!(*ins->codep & 0x10))
> > +	ins->rex2 |= REX_R;
> > +
> >        ins->rex = ~(*ins->codep >> 5) & 0x7;
> > -      ins->vex.r = *ins->codep & 0x10;
> > -      switch ((*ins->codep & 0xf))
> > +      switch ((*ins->codep & 0x7))
> 
> Please can you take the opportunity and drop the excess parentheses?
> 

Done.

> > @@ -9041,12 +9106,24 @@ get_valid_dis386 (const struct dis386 *dp,
> > instr_info *ins)
> >
> >        if (ins->address_mode != mode_64bit)
> >  	{
> > +	  if (ins->evex_type != evex_default
> > +	      || (ins->rex2 & (REX_B | REX_X)))
> > +	    return &bad_opcode;
> 
> What's special about X and B?
> 

For evex_default, the values of these two bits are fixed. Comment added.

      if (ins->address_mode != mode_64bit)
        {
          /* Report bad for !evex_default and when two fixed values of evex
             change..  */
          if (ins->evex_type != evex_default
              || (ins->rex2 & (REX_B | REX_X)))
            return &bad_opcode;

> > @@ -9460,6 +9537,13 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        dp = get_valid_dis386 (dp, &ins);
> >        if (dp == &err_opcode)
> >  	goto fetch_error_out;
> > +
> > +      /* For APX instructions promoted from legacy maps 0/1, prefix
> > +	 0x66 is interpreted as the operand size override.  */
> > +      if (ins.evex_type == evex_from_legacy
> > +	  && ins.vex.prefix == DATA_PREFIX_OPCODE)
> > +	sizeflag ^= DFLAG;
> 
> I think the comment wants to say "embedded prefix", as "prefix 0x66" is
> simply invalid to use with EVEX.
> 

Done, thanks.

> > @@ -9639,6 +9723,24 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        if (ins.last_repnz_prefix >= 0)
> >  	ins.all_prefixes[ins.last_repnz_prefix] = 0xf2;
> >        break;
> > +
> > +    case PREFIX_NP_OR_DATA:
> > +      if (ins.vex.prefix & ~DATA_PREFIX_OPCODE)
> 
> ~DATA_PREFIX_OPCODE == 0x99, which likely isn't what you mean here? Do
> you perhaps mean e.g. "> DATA_PREFIX_OPCODE"? (Using the opcodes in
> vex.prefix is questionable anyway, but that's a pre-existing oddity.)
> 

(A || 0) & ~A must be 0. It's hard to read.  

How about this ? This is more intuitive and easy to understand.

    case PREFIX_NP_OR_DATA:
      if (ins.vex.prefix == REPE_PREFIX_OPCODE
          || ins.vex.prefix == REPNE_PREFIX_OPCODE)
        {
          i386_dis_printf (info, dis_style_text, "(bad)");
          ret = ins.end_codep - priv.the_buffer;
          goto out;
        }

Thanks,
Lili.