RE: [PATCH V2] Support APX NF

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

> > @@ -10564,6 +10583,29 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  	    }
> >  	  else if (l == 1 && last[0] == 'C')
> >  	    break;
> > +	  else if (l == 1 && last[0] == 'N')
> > +	    {
> > +	      ins->has_nf = true;
> > +
> > +	      if (ins->vex.nf == true)
> > +		{
> > +		  *ins->obufp++ = '{';
> > +		  *ins->obufp++ = 'n';
> > +		  *ins->obufp++ = 'f';
> > +		  *ins->obufp++ = '}';
> > +		  *ins->obufp++ = ' ';
> > +		}
> > +	      else if (ins->evex_type == evex_from_legacy && !ins->vex.b)
> > +		{
> > +		  *ins->obufp++ = '{';
> > +		  *ins->obufp++ = 'e';
> > +		  *ins->obufp++ = 'v';
> > +		  *ins->obufp++ = 'e';
> > +		  *ins->obufp++ = 'x';
> > +		  *ins->obufp++ = '}';
> > +		  *ins->obufp++ = ' ';
> > +		}
> 
> This looks unrelated (by the title perhaps more related to the other patch you
> sent subsequently), and is also not tested anywhere if I'm not mistaken.
> I don't mind it being kept here, but besides needing at least minimal testing
> this is then also something that wants mentioning in the description (for going
> beyond the basic purpose of the patch). Plus this effect also needs mentioning
> above in the documentation of "NF".
> 
> Finally for both parts: Why not oappend() or even oappend_with_style()? I
> realize we have other similar pieces of code, but I question their correctness
> the latest after styling was added.
> 

Change it to oappend() and move it to another patch.

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

> >  inc, 0x40, No64, No_bSuf|No_sSuf|No_qSuf, { Reg16|Reg32 }  inc,
> > 0xfe/0, APX_F,
> W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF,
> > {Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64}
> > inc, 0xfe/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, {
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +inc, 0xfe/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> 
> ... pair of INC/DEC as well, despite it their number only going from 2 to 4.
> And quite certainly also for others below.
> 
> > @@ -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. I found that basically all instructions involving Acc are not extended, except SHA.

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

Thanks,
Lili.