Re: [PATCH 06/10] Support Intel RAO-INT

["H.J. Lu" <hjl.tools@gmail.com>]

On Sun, Oct 23, 2022 at 8:12 PM Jiang, Haochen <haochen.jiang@intel.com> wrote:
>
> > -----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.

Please also remove CPU_ANY_RAO_INT_FLAGS since it isn't used.

>
> >
> > > --- 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



-- 
H.J.