> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, October 14, 2022 10:38 PM
> To: Jiang, Haochen <haochen.jiang@intel.com>
> Cc: hjl.tools@gmail.com; Kong, Lingling <lingling.kong@intel.com>;
> binutils@sourceware.org
> Subject: Re: [PATCH 06/10] Support Intel RAO-INT
> 
> On 14.10.2022 11:12, Haochen Jiang wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -1097,7 +1097,8 @@ 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)
> > +  SUBARCH (cmpccxadd, CMPCCXADD, ANY_CMPCCXADD, false),  SUBARCH
> > + (raoint, RAOINT, ANY_RAOINT, false),
> 
> As for the earlier patch - likely no need for ANY_RAOINT. Also please have the
> earlier patch add the comma so you don't need to touch that line again here
> (helping at least "git blame").

Done and fixed for CMPccXADD patch.

> 
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -887,6 +887,7 @@ enum
> >    MOD_0F38F9,
> >    MOD_0F38FA_PREFIX_1,
> >    MOD_0F38FB_PREFIX_1,
> > +  MOD_0F38FC,
> >    MOD_0F3A0F_PREFIX_1,
> >
> >    MOD_VEX_0F12_PREFIX_0,
> > @@ -1086,6 +1087,7 @@ enum
> >    PREFIX_0F38F8,
> >    PREFIX_0F38FA,
> >    PREFIX_0F38FB,
> > +  PREFIX_0F38FC,
> 
> PREFIX_0F38FC_M_0 please (see comment on an earlier patch). However, like in
> the earlier patch - if you used Mdq below, you could avoid going through
> mod_table[] altogether.

Removed pass modrm table since Edq seems also judges modrm.

> 
> > @@ -3598,6 +3600,14 @@ static const struct dis386 prefix_table[][4] = {
> >      { MOD_TABLE (MOD_0F38FB_PREFIX_1) },
> >    },
> >
> > +  /* PREFIX_0F38FC */
> > +  {
> > +    { "aadd",	{ Edq, Gdq }, PREFIX_OPCODE },
> > +    { "axor",	{ Edq, Gdq }, PREFIX_OPCODE },
> > +    { "aand",	{ Edq, Gdq }, PREFIX_OPCODE },
> > +    { "aor",	{ Edq, Gdq }, PREFIX_OPCODE },
> > +  },
> 
> Once having gone through prefix_table[], PREFIX_OPCODE (and
> PREFIX_DATA) are meaningless iirc and should hence be omitted.
> 

Fixed.

> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> > @@ -3317,3 +3317,12 @@ cmpsxadd, 0x66e8, None, CpuCMPCCXADD|Cpu64,
> > Modrm|Vex128|Space0F38|VexVVVV=1|Swa
> >  cmpzxadd, 0x66e4, None, CpuCMPCCXADD|Cpu64,
> >
> Modrm|Vex128|Space0F38|VexVVVV=1|SwapSources|CheckRegSize|No_bSuf|
> No_w
> > Suf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32|Reg64, Reg32|Reg64,
> > Dword|Qword|Unspecified|BaseIndex }
> >
> >  // CMPCCXADD instructions end.
> > +
> > +// RAOINT instructions.
> 
> Nit: Better RAO-INT, like in the title?

Done.

> 
> > +aadd, 0xf38fc, None, CpuRAOINT,
> > +Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_ldSuf,{ Reg32|Reg64,
> > +Dword|Qword|Unspecified|BaseIndex}
> > +aand, 0x660f38fc, None, CpuRAOINT,
> > +Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_ldSuf,{ Reg32|Reg64,
> > +Dword|Qword|Unspecified|BaseIndex}
> > +aor, 0xf20f38fc, None, CpuRAOINT,
> > +Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_ldSuf,{ Reg32|Reg64,
> > +Dword|Qword|Unspecified|BaseIndex}
> > +axor, 0xf30f38fc, None, CpuRAOINT,
> > +Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_ldSuf,{ Reg32|Reg64,
> > +Dword|Qword|Unspecified|BaseIndex}
> 
> Why IgnoreSize? Instead I think you need CheckRegSize (assuming it does
> enough for Intel syntax memory operands - please double check; if not this will
> need fixing).
> 

For table, we aligned with CMPccXADD and added No_lSuf and No_qSuf since
the suffixes are not required.

In future, if suffixes are not required, we will add all the No_xxSuf.

BTW, can we write a macro named No_allSuf including all of them to shorten
the line?

Haochen
> Jan