Re: [PATCH 03/10] Support Intel AVX-NE-CONVERT

[Jan Beulich <jbeulich@suse.com>]

On 14.10.2022 11:12, Haochen Jiang wrote:
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -937,6 +937,8 @@ enum
>    MOD_VEX_0F385E_X86_64_P_3_W_0,
>    MOD_VEX_0F388C,
>    MOD_VEX_0F388E,
> +  MOD_VEX_0F38B0,
> +  MOD_VEX_0F38B1,
>    MOD_VEX_0F3A30_L_0,
>    MOD_VEX_0F3A31_L_0,
>    MOD_VEX_0F3A32_L_0,
> @@ -1132,6 +1134,9 @@ enum
>    PREFIX_VEX_0F3851_W_0,
>    PREFIX_VEX_0F385C_X86_64,
>    PREFIX_VEX_0F385E_X86_64,
> +  PREFIX_VEX_0F3872,
> +  PREFIX_VEX_0F38B0,
> +  PREFIX_VEX_0F38B1,

PREFIX_VEX_0F38B0_M_1_W_0 and PREFIX_VEX_0F38B1_M_1_W_0 respectively,
please. These enumerators are supposed to show the already decoded
components. Plus they need to be unambiguous if insns appear which
vary in (here) permitted ModR/M forms or the VEX.W bit.

> @@ -1526,8 +1531,11 @@ enum
>    VEX_W_0F385E_X86_64_P_1,
>    VEX_W_0F385E_X86_64_P_2,
>    VEX_W_0F385E_X86_64_P_3,
> +  VEX_W_0F3872_P_1,
>    VEX_W_0F3878,
>    VEX_W_0F3879,
> +  VEX_W_0F38B0,
> +  VEX_W_0F38B1,

VEX_W_0F38B0_M_1 and VEX_W_0F38B1_M_1 respectively, please.

> @@ -7618,6 +7654,14 @@ static const struct dis386 vex_w_table[][2] = {
>      /* VEX_W_0F3879 */
>      { "vpbroadcastw",	{ XM, EXw }, PREFIX_DATA },
>    },
> +  {
> +  /* VEX_W_0F38B0 */

Nit: Indentation.

> @@ -8428,6 +8472,14 @@ static const struct dis386 mod_table[][2] = {
>      /* MOD_VEX_0F388E */
>      { "vpmaskmov%DQ",	{ Mx, Vex, XM }, PREFIX_DATA },
>    },
> +  {
> +    /* MOD_VEX_0F38B0*/
> +    { VEX_W_TABLE (VEX_W_0F38B0) },
> +  },
> +  {
> +    /* MOD_VEX_0F38B1*/
> +    { VEX_W_TABLE (VEX_W_0F38B1) },
> +  },

Nit: Blanks missing in both of the comments.

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -249,6 +249,8 @@ static initializer cpu_flag_init[] =
>      "CPU_AVX2_FLAGS|CpuAVX_IFMA" },
>    { "CPU_AVX_VNNI_INT8_FLAGS",
>      "CPU_AVX2_FLAGS|CpuAVX_VNNI_INT8" },
> +  { "CPU_AVX_NE_CONVERT_FLAGS",
> +    "CPU_AVX2_FLAGS|CpuAVX_NE_CONVERT" },
>    { "CPU_IAMCU_FLAGS",
>      "Cpu186|Cpu286|Cpu386|Cpu486|Cpu586|CpuIAMCU" },
>    { "CPU_ADX_FLAGS",
> @@ -447,6 +449,8 @@ static initializer cpu_flag_init[] =
>      "CpuAVX_IFMA" },
>    { "CPU_ANY_AVX_VNNI_INT8_FLAGS",
>      "CpuAVX_VNNI_INT8" },
> +  { "CPU_ANY_AVX_NE_CONVERT_FLAGS",
> +    "CpuAVX_NE_CONVERT" },
>  };

Like for patches 1 and 2 - CPU_ANY_AVX2_FLAGS also need adjusting.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3027,6 +3027,21 @@ movdir64b, 0x660f38f8, None, CpuMOVDIR64B, Modrm|AddrPrefixOpReg, { Unspecified|
>  
>  // MOVEDIR instructions end.
>  
> +// AVX_NE_CONVERT instructions.
> +
> +vbcstnebf162ps, 0xf3b1, None, CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Word|Unspecified|BaseIndex, RegXMM|RegYMM}
> +vbcstnesh2ps,   0x66b1, None, CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Word|Unspecified|BaseIndex, RegXMM|RegYMM}

Why CheckRegSize?

> +vcvtneoph2ps,   0xb0,   None,   CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Xmmword|Ymmword|Unspecified|BaseIndex, RegXMM|RegYMM}
> +vcvtneebf162ps, 0xf3b0, None,   CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Xmmword|Ymmword|Unspecified|BaseIndex, RegXMM|RegYMM}
> +vcvtneeph2ps,   0x66b0, None,   CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Xmmword|Ymmword|Unspecified|BaseIndex, RegXMM|RegYMM}
> +vcvtneobf162ps, 0xf2b0, None,   CpuAVX_NE_CONVERT, Modrm|Vex|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Xmmword|Ymmword|Unspecified|BaseIndex, RegXMM|RegYMM}
> +vcvtneps2bf16,  0xf372, None,   CpuAVX_NE_CONVERT, Modrm|PseudoVexPrefix|Vex128|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Xmmword|RegXMM|Unspecified|BaseIndex, RegXMM}
> +vcvtneps2bf16,  0xf372, None,   CpuAVX_NE_CONVERT, Modrm|PseudoVexPrefix|Vex256|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, {Ymmword|RegYMM|Unspecified|BaseIndex, RegXMM}
> +vcvtneps2bf16x, 0xf372, None,   CpuAVX_NE_CONVERT, Modrm|PseudoVexPrefix|Vex128|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ATTSyntax, {Xmmword|RegXMM|Unspecified|BaseIndex, RegXMM}
> +vcvtneps2bf16y,  0xf372, None,   CpuAVX_NE_CONVERT, Modrm|PseudoVexPrefix|Vex256|Space0F38|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|ATTSyntax, {Ymmword|RegYMM|Unspecified|BaseIndex, RegXMM}

Nit: Excess blank after the first comma on the last of the above lines, and
excess blanks on all vcvt* lines before CpuAVX_NE_CONVERT.

Unnecessary Xmmword / Ymmword on the last four lines. And there again - why
CheckRegSize? But these would anyway benefit from using the AVX instance of
the <xy> template - then AT&T syntax handling for the suffix-less variant
would automatically be correct (it isn't in your code). Problem is that it
wasn't clear that there'll be a need to re-use xy's AVX instance this late
in the file, so the overloading of the name (commit e07ae9a3efee) will need
undoing. To keep names short (see commit 390ddd6f6812) I'd recommend using
Vxy and Exy (for the VEX and EVEX variants respectively).

Like for the earlier patches I disagree with the uses of PseudoVexPrefix
here (as said - the attribute should really go away again, and I have a
patch doing so), but then ...

> +
> +// AVX_NE_CONVERT instructions end.
> +
>  // AVX512_BF16 instructions.
>  
>  vcvtne2ps2bf16, 0xf272, None, CpuAVX512_BF16, Modrm|Space0F38|VexVVVV|Masking=3|VexW0|Broadcast|Disp8ShiftVL|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegXMM|RegYMM|RegZMM|Dword|Unspecified|BaseIndex, RegXMM|RegYMM|RegZMM, RegXMM|RegYMM|RegZMM }

... the new insns (at least the ones having AVX512 counterparts) need to be
placed after the AVX512 ones (which you'll note use the <xy> template.

Jan