[PATCH 04/10] Support Intel CMPccXADD

[Jan Beulich]

On 14.10.2022 11:12, Haochen Jiang wrote:
> --- a/gas/NEWS
> +++ b/gas/NEWS
> @@ -1,5 +1,7 @@
>  -*- text -*-
>  
> +* Add support for Intel CMPccXADD instructions.
> +
>  * Add support for Intel AVX-NE-CONVERT instructions.
>  
>  * Add support for Intel AVX-VNNI-INT8 instructions.

I wonder if all of these really need a separate line.

> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -1097,6 +1097,7 @@ static const arch_entry cpu_arch[] =
>    SUBARCH (avx_ifma, AVX_IFMA, ANY_AVX_IFMA, false),
>    SUBARCH (avx_vnni_int8, AVX_VNNI_INT8, ANY_AVX_VNNI_INT8, false),
>    SUBARCH (avx_ne_convert, AVX_NE_CONVERT, ANY_AVX_NE_CONVERT, false),
> +  SUBARCH (cmpccxadd, CMPCCXADD, ANY_CMPCCXADD, false)
>  };

No need for ANY_CMPCCXADD, unless you _know_ dependent features will appear.
See e.g. FSGSBASE, i.e. you can use CMPCCXADD twice here.

> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -366,6 +366,7 @@ fetch_data (struct disassemble_info *info, bfd_byte *addr)
>  #define Ma { OP_M, a_mode }
>  #define Mb { OP_M, b_mode }
>  #define Md { OP_M, d_mode }
> +#define Mdq { OP_M, dq_mode }

You're decoding via mod_table[], so I don't think you need this. Or (perhaps
better) vice versa - keep this (if there's no pre-existing one that fits) and
avoid the decode step through mod_table[].

> @@ -939,6 +940,22 @@ enum
>    MOD_VEX_0F388E,
>    MOD_VEX_0F38B0,
>    MOD_VEX_0F38B1,
> +  MOD_VEX_0F38E0_X86_64,
> +  MOD_VEX_0F38E1_X86_64,
> +  MOD_VEX_0F38E2_X86_64,
> +  MOD_VEX_0F38E3_X86_64,
> +  MOD_VEX_0F38E4_X86_64,
> +  MOD_VEX_0F38E5_X86_64,
> +  MOD_VEX_0F38E6_X86_64,
> +  MOD_VEX_0F38E7_X86_64,
> +  MOD_VEX_0F38E8_X86_64,
> +  MOD_VEX_0F38E9_X86_64,
> +  MOD_VEX_0F38EA_X86_64,
> +  MOD_VEX_0F38EB_X86_64,
> +  MOD_VEX_0F38EC_X86_64,
> +  MOD_VEX_0F38ED_X86_64,
> +  MOD_VEX_0F38EE_X86_64,
> +  MOD_VEX_0F38EF_X86_64,

Hmm, I really need to split off (and re-submit) the re-usable parts of
"x86-64: Intel64 adjustments for conditional jumps" (see
https://sourceware.org/pipermail/binutils/2020-July/112365.html), to avoid
the need for 16 almost identical entries of several kinds throughout this
patch.

> @@ -8480,6 +8609,70 @@ static const struct dis386 mod_table[][2] = {
>      /* MOD_VEX_0F38B1*/
>      { VEX_W_TABLE (VEX_W_0F38B1) },
>    },
> +  {
> +    /* MOD_VEX_0F38E0_X86_64 */
> +    { "cmpoxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },
> +  },
> +  {
> +    /* MOD_VEX_0F38E1_X86_64 */
> +    { "cmpnoxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },
> +  },
> +  {
> +    /* MOD_VEX_0F38E2_X86_64 */
> +    { "cmpbxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },
> +  },
> +  {
> +    /* MOD_VEX_0F38E3_X86_64 */
> +    { "cmpnbxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },

I understand the ISA extensions document names the insn this way and doesn't
list cmpaexadd (same for other aliases), but I think this is a mistake in
the doc. I've raised a respective question in the ISA extensions forum: I
think representation of conditions to check for should be uniform among
insns, and hence it should be "ae" here. (That would also be the effect if
you used %C<whatever> here.)

> @@ -433,7 +436,7 @@ typedef union i386_cpu_flags
>        unsigned int cpu64:1;
>        unsigned int cpuno64:1;
>  #ifdef CpuUnused
> -      unsigned int unused:(CpuNumOfBits - CpuUnused);
> +      // unsigned int unused:(CpuNumOfBits - CpuUnused);
>  #endif

No - you should instead comment out the #define of CpuUnused - see the
comment there.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3296,3 +3296,24 @@ vpdpbsud, 0xf350, None, CpuAVX_VNNI_INT8, Modrm|Vex|Space0F38|VexVVVV|VexW0|Chec
>  vpdpbsuds, 0xf351, None, CpuAVX_VNNI_INT8, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
>  
>  // AVX_VNNI_INT8 instructions end.
> +
> +// CMPCCXADD instructions.
> +
> +cmpbexadd, 0x66e6, None, CpuCMPCCXADD|Cpu64, Modrm|Vex128|Space0F38|VexVVVV=1|SwapSources|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32|Reg64, Reg32|Reg64, Dword|Qword|Unspecified|BaseIndex }

Along the lines of the earlier comment - you want to use the <cc>
template here, eliminating the need for 16 almost identical lines _and_
supplying all condition code representation in one go.

Apart from that you forgot CheckRegSize here afaict. And please again
VexVVVV alone, without =1. Also for non-vector insns perhaps better plain
Vex instead of Vex128. Further these insns should allow for l and q
suffixes in AT&T mode.

And finally - is SwapSources really appropriate to use here? There's only
one pure source operand, the other two are also serving as destinations.
I wonder whether an attribute is necessary here in the first place: Vex-
encoded insns with a memory destination never have two further register
operands, so that property should suffice for identifying the case in
build_modrm_byte(). Alternatively you could also simply use the CPU flag.

Jan