RE: [PATCH V2] Support APX NF

["Cui, Lili" <lili.cui@intel.com>]

> On 12.03.2024 14:22, Cui, Lili wrote:
> >>> --- a/opcodes/i386-opc.tbl
> >>> +++ b/opcodes/i386-opc.tbl
> >>> @@ -310,32 +310,42 @@ sti, 0xfb, 0, NoSuf, {}  // Arithmetic.
> >>>  add, 0x0, APX_F,
> >>> D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, {
> >>> Reg8|Reg16|Reg32|Reg64,
> >> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> >>> Reg8|Reg16|Reg32|Reg64 }  add, 0x0, 0,
> >>> D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, {
> >>> Reg8|Reg16|Reg32|Reg64,
> >> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>> +add, 0x0, APX_F,
> >> D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, {
> >>> +Reg8|Reg16|Reg32|Reg64,
> >> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> >>> +}
> >>>  add, 0x83/0, APX_F,
> >>> Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4|NF,
> >> { Imm8S,
> >>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> add,
> >>> 0x83/0, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S,
> >>> Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>> +add, 0x83/0, APX_F, Modrm|No_bSuf|No_sSuf|EVexMap4|NF,
> { Imm8S,
> >>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>>  add, 0x4, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S,
> >>> Acc|Byte|Word|Dword|Qword }  add, 0x80/0, APX_F,
> >>> W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVexMap4|NF, {
> >>> Imm8|Imm16|Imm32|Imm32S,
> >> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> >>> Reg8|Reg16|Reg32|Reg64}  add, 0x80/0, 0,
> >>> W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S,
> >>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>> +add, 0x80/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> >>> +Imm8|Imm16|Imm32|Imm32S,
> >> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> >>> +}
> >>
> >> As before, this (and similar new ones) can also be picked up without
> >> use of {nf}, which will want testing. If you deliberately defer
> >> adding such a test, this needs saying in the description.
> >>
> >
> > Ok.
> >
> >> One further remark: In the course of adding APX support the set of
> >> templates for ADD and its sibling ALU ops was bumped from 4 to 10.
> >> There was quite a bit of redundancy between these 7 groups before,
> >> but it's growing much worse now. Did you consider templatizing them
> >> up front? This may even be worthwhile for just the ...
> >>
> >
> > I try to add something like this, but it doesn't work. Seems that it only
> supports differentiation based on mnemonic.
> >
> > +<cpu1:attr, 0:HLEPrefixLock, APX_F:EVexMap4|NF> <cpu2:attr,
> > +0:HLEPrefixLock|Optimize, APX_F:EVexMap4|NF>
> > +
> >  // Arithmetic.
> >  add, 0x0, APX_F,
> > D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> > Reg8|Reg16|Reg32|Reg64 } -add, 0x0, 0,
> > D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -add, 0x0, APX_F,
> D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +add<cpu1>, 0x0, 0,
> D|W|CheckOperandSize|Modrm|No_sSuf|<cpu1:attr>, {
> > +Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> > +}
> 
> Differentiating on other than the mnemonic is possible (see e.g. the mmx and
> sse templates, which append nothing to the mnemonics) but not wanted
> here. The goal of the suggested template is to have something that covers all 7
> mnemonics in one go, including their legacy forms.
> 

The handling of <mmx/<sse are clever. The current templates are indeed redundant. Do you mind I create a separate patch to clean up all APX templates instead of in this patch.

> >>> @@ -434,24 +464,34 @@ imul, 0x69, i186,
> >>> Modrm|No_bSuf|No_sSuf|RegKludge, { Imm16|Imm32|Imm32S,
> >> Reg16|R
> >>>
> >>>  div, 0xf6/6, 0, W|Modrm|No_sSuf, {
> >>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }  div, 0xf6/6, 0,
> >>> W|CheckOperandSize|Modrm|No_sSuf, {
> >>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> >>> Acc|Byte|Word|Dword|Qword }
> >>> +div, 0xf6/6, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> >>> +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>>  idiv, 0xf6/7, 0, W|Modrm|No_sSuf, {
> >>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }  idiv, 0xf6/7, 0,
> >>> W|CheckOperandSize|Modrm|No_sSuf, {
> >>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> >>> Acc|Byte|Word|Dword|Qword }
> >>> +idiv, 0xf6/7, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> >>> +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>
> >> What about the secondary forms with an explicit destination operand?
> >
> > The doc does not extend the second format. Maybe it is related to Acc.
> It certainly is.
> 
> > I found that basically all instructions involving Acc are not extended, except
> SHA.
> 
> Well, the SDM doesn't specify DIV or IDIV with a second operand.
> Therefore the issue here isn't conformance with the documentation, but
> consistency with the pre-existing gas extensions.
>
 
Yes, it is special, and we should extend the second format.

> >>> @@ -2027,6 +2090,7 @@ blsi, 0xf3/3, APX_F(BMI),
> >>> Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVV
> >>>  blsmsk, 0xf3/2, APX_F(BMI),
> >>>
> >>
> Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSu
> >> f|No_wSu
> >>> f|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> >>> blsr, 0xf3/1, APX_F(BMI),
> >>>
> >>
> Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSu
> >> f|No_wSu
> >>> f|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> >>> tzcnt, 0xf30fbc, BMI, Modrm|CheckOperandSize|No_bSuf|No_sSuf, {
> >>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>> +tzcnt, 0xf4, BMI&APX_F,
> >>> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, {
> >>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>
> >> Like e.g. here, ...
> >>
> >>> @@ -2104,9 +2168,11 @@ insertq, 0xf20f78, SSE4a, Modrm|NoSuf,
> >> { Imm8,
> >>> Imm8, RegXMM, RegXMM }
> >>>
> >>>  // LZCNT instruction
> >>>  lzcnt, 0xf30fbd, LZCNT, Modrm|CheckOperandSize|No_bSuf|No_sSuf, {
> >>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>> +lzcnt, 0xf5, LZCNT|APX_F,
> >>> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, {
> >>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>
> >> ... I think you mean LZCNT&APX_F here.
> >>
> >>>  // POPCNT instruction
> >>>  popcnt, 0xf30fb8, POPCNT,
> Modrm|CheckOperandSize|No_bSuf|No_sSuf,
> >> {
> >>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>> +popcnt, 0x88, POPCNT|APX_F,
> >>> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, {
> >>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> >>
> >> Whereas here it ought to be just APX_F if the doc is to be trusted.
> >
> > I think they should be LZCNT&APX_F and POPCNT&APX_F, why do you think
> POPCNT is special? I suspect I've missed something.
> 
> Unless you're looking at a newer version of the APX spec than me, you'll
> observe the difference when looking at the respective insn
> pages: For LZCNT both CPUID bits are mentioned, while for POPCNT only
> APX_F is there.
> 
You are right. I will double confirm it with doc.

Thanks,
Lili.