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

[Jan Beulich <jbeulich@suse.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?

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

> @@ -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?

> @@ -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?

> @@ -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 ..."?

> +	 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)?

> @@ -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?

> @@ -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?

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

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

Jan