Re: [PATCH 1/5] Support Intel AVX-VNNI-INT16

[Jan Beulich <jbeulich@suse.com>]

On 13.07.2023 08:32, Haochen Jiang wrote:
> @@ -3909,7 +3913,21 @@ static const struct dis386 prefix_table[][4] = {
>      { "vbcstnebf162ps", { XM, Mw }, 0 },
>      { "vbcstnesh2ps", { XM, Mw }, 0 },
>    },
> - 
> +  
> +  /* PREFIX_VEX_0F38D2 */

This and ...

> +  {
> +    { "vpdpwuud",	{ XM, Vex, EXx }, 0 },
> +    { "vpdpwsud",	{ XM, Vex, EXx }, 0 },
> +    { "vpdpwusd",	{ XM, Vex, EXx }, 0 },
> +  },
> +
> +  /* PREFIX_VEX_0F38D3 */

... this comment want to mention the correct enumerator names.

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -166,6 +166,8 @@ static const dependency isa_dependencies[] =
>      "AVX2" },
>    { "FRED",
>      "LKGS" },
> +  { "AVX_VNNI_INT16",
> +    "AVX2" },

Can this please be moved up ahead of FRED, perhaps immediately after
AVX_VNNI_INT8?

> @@ -366,6 +368,7 @@ static bitfield cpu_flags[] =
>    BITFIELD (RAO_INT),
>    BITFIELD (FRED),
>    BITFIELD (LKGS),
> +  BITFIELD (AVX_VNNI_INT16),

While not as relevant here, moving up would be nice in this case as well.

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -233,6 +233,8 @@ enum
>    CpuFRED,
>    /* lkgs instruction required */
>    CpuLKGS,
> +  /* Intel AVX VNNI-INT16 Instructions support required.  */
> +  CpuAVX_VNNI_INT16,
>    /* mwaitx instruction required */
>    CpuMWAITX,
>    /* Clzero instruction required */
> @@ -430,6 +432,7 @@ typedef union i386_cpu_flags
>        unsigned int cpurao_int:1;
>        unsigned int cpufred:1;
>        unsigned int cpulkgs:1;
> +      unsigned int cpuavx_vnni_int16:1;
>        unsigned int cpumwaitx:1;
>        unsigned int cpuclzero:1;
>        unsigned int cpuospke:1;

Adjustments to this file may then also be needed.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3364,3 +3364,14 @@ erets, 0xf20f01ca, FRED|x64, NoSuf, {}
>  eretu, 0xf30f01ca, FRED|x64, NoSuf, {}
>  
>  // FRED instructions end.
> +
> +// AVX_VNNI_INT16 instructions.
> +
> +vpdpwuud, 0xd2, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpwuuds, 0xd3, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpwusd, 0x66d2, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpwusds, 0x66d3, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpwsud, 0xf3d2, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpwsuds, 0xf3d3, AVX_VNNI_INT16, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +
> +// AVX_VNNI_INT16 instructions end.

While purely cosmetic here, I think it is a bad habit to always add to
the bottom of the file. Not only does this result in related entries
sometimes being far apart, but it also increases the risk of conflicts
between patches. Therefore I'd like to ask to put this next to
AVX-VNNI-INT8 as well (and note the dashes used there, which you will
want to use here as well).

Okay with all of these adjustments. In case you disagree with any of
the requests, please submit a v2.

Jan