Re: [PATCH 09/10] Support Intel AMX-FP16

[Jan Beulich <jbeulich@suse.com>]

On 14.10.2022 11:12, Haochen Jiang wrote:
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -933,6 +933,7 @@ enum
>    MOD_VEX_0F384B_X86_64_P_3_W_0,
>    MOD_VEX_0F385A,
>    MOD_VEX_0F385C_X86_64_P_1_W_0,
> +  MOD_VEX_0F385C_X86_64_P_3_W_0,
>    MOD_VEX_0F385E_X86_64_P_0_W_0,
>    MOD_VEX_0F385E_X86_64_P_1_W_0,
>    MOD_VEX_0F385E_X86_64_P_2_W_0,
> @@ -1399,6 +1400,7 @@ enum
>    VEX_LEN_0F384B_X86_64_P_3_W_0_M_0,
>    VEX_LEN_0F385A_M_0,
>    VEX_LEN_0F385C_X86_64_P_1_W_0_M_0,
> +  VEX_LEN_0F385C_X86_64_P_3_W_0_M_0,
>    VEX_LEN_0F385E_X86_64_P_0_W_0_M_0,
>    VEX_LEN_0F385E_X86_64_P_1_W_0_M_0,
>    VEX_LEN_0F385E_X86_64_P_2_W_0_M_0,
> @@ -1565,6 +1567,7 @@ enum
>    VEX_W_0F3859,
>    VEX_W_0F385A_M_0_L_0,
>    VEX_W_0F385C_X86_64_P_1,
> +  VEX_W_0F385C_X86_64_P_3,
>    VEX_W_0F385E_X86_64_P_0,
>    VEX_W_0F385E_X86_64_P_1,
>    VEX_W_0F385E_X86_64_P_2,
> @@ -4088,6 +4091,7 @@ static const struct dis386 prefix_table[][4] = {
>      { Bad_Opcode },
>      { VEX_W_TABLE (VEX_W_0F385C_X86_64_P_1) },
>      { Bad_Opcode },
> +    { VEX_W_TABLE (VEX_W_0F385C_X86_64_P_3) },
>    },
>  
>    /* PREFIX_VEX_0F385E_X86_64 */
> @@ -7120,6 +7124,11 @@ static const struct dis386 vex_len_table[][2] = {
>      { "tdpbf16ps", { TMM, EXtmm, VexTmm }, 0 },
>    },
>  
> +  /* VEX_LEN_0F385C_X86_64_P_3_W_0_M_0 */
> +  {
> +    { "tdpfp16ps", { TMM, EXtmm, VexTmm }, 0 },
> +  },
> +
>    /* VEX_LEN_0F385E_X86_64_P_0_W_0_M_0 */
>    {
>      { "tdpbuud", {TMM, EXtmm, VexTmm }, 0 },
> @@ -7788,6 +7797,10 @@ static const struct dis386 vex_w_table[][2] = {
>      /* VEX_W_0F385C_X86_64_P_1 */
>      { MOD_TABLE (MOD_VEX_0F385C_X86_64_P_1_W_0) },
>    },
> +  {
> +    /* VEX_W_0F385C_X86_64_P_3 */
> +    { MOD_TABLE (MOD_VEX_0F385C_X86_64_P_3_W_0) },
> +  },
>    {
>      /* VEX_W_0F385E_X86_64_P_0 */
>      { MOD_TABLE (MOD_VEX_0F385E_X86_64_P_0_W_0) },
> @@ -8610,6 +8623,11 @@ static const struct dis386 mod_table[][2] = {
>      { Bad_Opcode },
>      { VEX_LEN_TABLE (VEX_LEN_0F385C_X86_64_P_1_W_0_M_0) },
>    },
> +  {
> +    /* MOD_VEX_0F385C_X86_64_P_3_W_0 */
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F385C_X86_64_P_3_W_0_M_0) },
> +  },
>    {
>      /* MOD_VEX_0F385E_X86_64_P_0_W_0 */
>      { Bad_Opcode },
> diff --git a/opcodes/i386-gen.c b/opcodes/i386-gen.c
> index eac229e54d..d10b462548 100644
> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -259,6 +259,8 @@ static initializer cpu_flag_init[] =
>      "CpuWRMSRNS" },
>    { "CPU_MSRLIST_FLAGS",
>      "CpuMSRLIST" },
> +  { "CPU_AMX_FP16_FLAGS",
> +    "CpuAMX_FP16" },
>    { "CPU_IAMCU_FLAGS",
>      "Cpu186|Cpu286|Cpu386|Cpu486|Cpu586|CpuIAMCU" },
>    { "CPU_ADX_FLAGS",

Can you please insert next to the other similar AMX entries? Seeing the flaw
here, I'll be making a patch to address the lack of CPU_AMX_TILE_FLAGS in
the similar pre-existing entries. When you move the insertion, it'll be
easier to keep things in sync.

> @@ -426,7 +428,7 @@ static initializer cpu_flag_init[] =
>    { "CPU_ANY_AMX_BF16_FLAGS",
>      "CpuAMX_BF16" },
>    { "CPU_ANY_AMX_TILE_FLAGS",
> -    "CpuAMX_TILE|CpuAMX_INT8|CpuAMX_BF16" },
> +    "CpuAMX_TILE|CpuAMX_INT8|CpuAMX_BF16|CpuAMX_FP16" },
>    { "CPU_ANY_AVX_VNNI_FLAGS",
>      "CpuAVX_VNNI" },
>    { "CPU_ANY_MOVDIRI_FLAGS",
> @@ -467,6 +469,8 @@ static initializer cpu_flag_init[] =
>      "CpuWRMSRNS" },
>    { "CPU_ANY_MSRLIST_FLAGS",
>      "CpuMSRLIST" },
> +  { "CPU_ANY_AMX_FP16_FLAGS",
> +    "CpuAMX_FP16" },
>  };

Same here then.

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -223,6 +223,8 @@ enum
>    CpuWRMSRNS,
>    /* Intel MSRLIST Instructions support required.  */
>    CpuMSRLIST,
> +  /* AMX-FP16 instructions required */
> +  CpuAMX_FP16,

This (and the related stuff) may also benefit from grouping with the other
AMX ones.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3339,3 +3339,9 @@ rdmsrlist, 0xf20f01c6, None, CpuMSRLIST|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|N
>  wrmsrlist, 0xf30f01c6, None, CpuMSRLIST|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {}
>  
>  // MSRLIST instructions end.
> +
> +// AMX-FP16 instructions.
> +
> +tdpfp16ps, 0xf25c, None, CpuAMX_FP16|Cpu64, Modrm|Vex128|Space0F38|VexVVVV=1|VexW0|SwapSources|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegTMM, RegTMM, RegTMM }

As before - plain VexVVVV preferably (without the =1), irrespective of the
already present AMX entries still using the less preferred form.

> +// AMX-FP16 instructions end.

Nit (again): Perhaps better use singular?

And as above - perhaps put next to the other AMX entries? Note how they
are all in a single group, despite it being 3 separate feature bits. So
I guess you will want to insert exactly one line below tdpbf16ps. That
way the similarity between both is also going to be easiest to see,
check, and maintain.

Jan