[PATCH v3 6/9] Support APX NDD

[Cui, Lili]

> >>> +add, 0x0, APX_F,
> +D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVex128|EVexMap4|N
> >> F, {
> >>> +Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>
> >> There is _still_ Byte|Word|Dword|Qword in here (and below), when I
> >> think I pointed out more than once before that in new templates such
> >> redundancy wants omitting.
> >>
> >> Since this isn't the first instance of earlier review comments not
> >> taken care of, may I please ask that you make reasonably sure that
> >> new versions aren't sent out like this?
> >>
> >
> > This part could indeed be omitted, but I really don't remember you
> mentioning it on the APX patches.
> 
> Already in e.g.
> https://sourceware.org/pipermail/binutils/2023-November/130422.html
> I pointed out that such earlier comments in e.g.
> https://sourceware.org/pipermail/binutils/2023-September/129590.html
> were not addressed.
> 

Sorry, movbe was indeed caused by the reg I added, I didn't notice that the leagcy template have this issue as well.  when you said I had something need to change, I didn't realize it was here at all.

> > There are still a lot of redundant Byte|Word|Dword|Qword in the opcode
> table, APX just added some flags on top of the old ones. Do you mind if I
> create a patch first to remove the redundant parts of master?
> 
> I don't mind you cleaning up first. It's just that normally I wouldn't do so in a
> separate patch (one of the reasons being that such non-functional changes get
> in the way of using "git blame" or alike when trying to find the most recent
> real change to a line), unless it was only a handful of instances left. Instead I
> typically do such tidying as lines are touched anyway. Thing here simply is that
> new templates shouldn't have such anomalies anymore.
> 

I still want to change them. It's easy to be misled.

> +D|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVex128|EVexMap4|Opti
> >> mize|
> >>> +NF, { Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64, }
> >>
> >> Here and elsewhere, what's Optimize for? It not being there on other
> >> templates, it can't be for the EVEX->REX2 optimization? If there are
> >> further optimization plans, that's (again) something to mention in
> >> the description. Yet better would be if such attributes were added
> >> only when respective optimizations are actually introduced. Unlike
> >> e.g. NF, which would mean another bulk update if not added right
> >> away, new optimizations typically affect only a few templates at a time.
> >>
> >
> > Optimize is not new.
> >
> > sub, 0x28, APX_F,
> >
> D|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVex128|EVexMap4|Opti
> mize|N
> > F, { Reg8|Reg16|Reg32|Reg64,
> >
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> > Reg8|Reg16|Reg32|Reg64, } sub, 0x28, 0,
> > D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock|Optimize, {
> > Reg8|Reg16|Reg32|Reg64,
> >
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> }
> 
> Optimize is legitimately there for the legacy template. If the new template also
> wants it, there needs to be some reason. Otherwise it is part of the
> tranformation to APX/EVEX to drop it.
> 

Dropped Optimize, thanks.

> >>>  sub, 0x28, 0,
> >>> D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock|Optimize, {
> >>> Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>> +sub, 0x83/5, APX_F,
> >>> +Modrm|No_bSuf|No_sSuf|DstVVVV|EVex128|EVexMap4|NF, { Imm8S,
> >>> +Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex,
> >>> +Reg16|Reg32|Reg64 }
> >>>  sub, 0x83/5, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S,
> >>> Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex }  sub,
> >> 0x2c,
> >>> 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S,
> >> Acc|Byte|Word|Dword|Qword }
> >>> +sub, 0x80/5, APX_F,
> >>>
> +W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVex128|EVexMap4|NF, {
> >>> +Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>>  sub, 0x80/5, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >>> Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>
> >> There are still only 3 new templates here (and also above for add,
> >> plus for other similar insns), when ...
> >>
> >>>  dec, 0x48, No64, No_bSuf|No_sSuf|No_qSuf, { Reg16|Reg32 }
> >>> +dec, 0xfe/1, APX_F,
> >>>
> +W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVex128|EVexMap4|NF, {
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>>  dec, 0xfe/1, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>>
> >>> +sbb, 0x18, APX_F,
> >>> +D|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVex128|EVexMap4,
> {
> >>> +Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>>  sbb, 0x18, 0, D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, {
> >>> Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>> +sbb, 0x18, APX_F,
> >>> +D|W|CheckOperandSize|Modrm|EVex128|EVexMap4|No_sSuf, {
> >>> +Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x }
> >>> +sbb, 0x83/3, APX_F,
> >>>
> >>
> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVex128|EVexMap4,
> >> {
> >>> +Imm8S,
> Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex,
> >>> +Reg16|Reg32|Reg64 }
> >>>  sbb, 0x83/3, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S,
> >>> Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex }
> >>> +sbb, 0x83/3, APX_F, Modrm|EVex128|EVexMap4|No_bSuf|No_sSuf,
> >> { Imm8S,
> >>> +Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex }
> >>>  sbb, 0x1c, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S,
> >>> Acc|Byte|Word|Dword|Qword }
> >>> +sbb, 0x80/3, APX_F,
> >>> +W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVex128|EVexMap4, {
> >>> +Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>>  sbb, 0x80/3, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >>> Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>> +sbb, 0x80/3, APX_F, W|Modrm|EVex128|EVexMap4|No_sSuf, {
> >>> +Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x }
> >>
> >> ... there are 6 new templates here. This is again an aspect I had
> >> pointed out before. You cannot defer the addition of the other 3
> >> until the NF patch, as you want to make sure that with just this
> >> patch in place something both
> >>
> >>     {evex} sbb %eax, %eax
> >>
> >> and
> >>
> >>     {evex} sub %eax, %eax
> >>
> >> actually assemble, and to EVEX encodings. I can't see how that would
> >> work in the latter case without those further templates.
> >>
> >> The alternative is to also defer adding the 2-operand SBB templates
> >> (and any others you add here which don't use DstVVVV).
> >>
> >
> > I'm having a headache with this, some instructions like sbb don't support NF,
> originally they were in the 4/9 patch, but their disassemblers are in the NDD
> patch, and you agreed to put them in the NDD patch.
> 
> Right, yet still the overall result wants to be consistent. Hence why I'm not
> demanding that you move these templates yet later (which is one option).
> Instead I've indicated that moving the others ahead would also be okay.
> 
 
I'd like to move them into the NF patch, only need to move the templates.  However, the second method is more cumbersome and requires moving the encoder, decoder, and test cases to the evex egpr patch.

> Like with any series, you want it to be in a shape where it can be committed
> piecemeal. Which is even more important with a release around the corner.
> If we end up with just partial APX support in 2.42, that partial support should
> be in a shape that's predictable to users.
> 
> > Now I really don't know where to move. Moving encoding, decoding, and
> especially test cases for instructions between patches is cumbersome and I
> really don't think it makes much sense.
> 
> I can see your point, and I'm sorry for the hassle. Part of the problem of the
> moving being troublesome is (imo) that many of the patches simply were
> (are) doing too many things at a time anyway.
> 
> >>>  xor, 0x30, 0,
> >>> D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock|Optimize, {
> >>> Reg8|Reg16|Reg32|Reg64,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>> +xor, 0x83/6, APX_F,
> >>>
> >>
> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVex128|EVexMap4|
> >> NF, {
> >>> +Imm8S,
> Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex,
> >>> +Reg16|Reg32|Reg64 }
> >>>  xor, 0x83/6, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S,
> >>> Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex }  xor,
> >> 0x34,
> >>> 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S,
> >> Acc|Byte|Word|Dword|Qword }
> >>> +xor, 0x80/6, APX_F,
> >>>
> +W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVex128|EVexMap4|NF, {
> >>> +Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInde
> >> x,
> >>> +Reg8|Reg16|Reg32|Reg64 }
> >>>  xor, 0x80/6, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> >>> Imm8|Imm16|Imm32|Imm32S,
> >>>
> >>
> Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex
> >> }
> >>>
> >>>  // clr with 1 operand is really xor with 2 operands.
> >>>  clr, 0x30, 0, W|Modrm|No_sSuf|RegKludge|Optimize, {
> >>> Reg8|Reg16|Reg32|Reg64 }
> >>
> >> Btw., for consistency this may also want accompanying with an EVEX
> >> counterpart.
> >>
> >
> > Do you mean to add an entry like this? It should belong to the previous
> patch.
> >
> > // clr with 1 operand is really xor with 2 operands.
> > clr, 0x30, 0, W|Modrm|No_sSuf|RegKludge|Optimize, {
> > Reg8|Reg16|Reg32|Reg64 } clr, 0x30, APX_F,
> > W|Modrm|No_sSuf|RegKludge|EVex128|EVexMap4|Optimize, {
> > Reg8|Reg16|Reg32|Reg64 }
> 
> Yes, something like this. And possibly indeed not the patch here; the template
> simply happened to be in context. Where exactly it wants to go depends - see
> above - on where other similar templates are introduced. Note however that
> the corresponding XOR templates are introduced here, just above and still in
> context.
> 

For clr's evex format template, I think it should be in the NF patch, since xor's evex format template is also in that patch.

Thanks,
Lili.