RE: [PATCH] Support APX PUSHP/POPP

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

> On 27.11.2023 13:31, Cui, Lili wrote:
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -1931,23 +1931,23 @@ static const struct dis386 dis386[] = {
> >    { "dec{S|}",		{ RMeSI }, 0 },
> >    { "dec{S|}",		{ RMeDI }, 0 },
> >    /* 50 */
> > -  { "push{!P|}",		{ RMrAX }, 0 },
> > -  { "push{!P|}",		{ RMrCX }, 0 },
> > -  { "push{!P|}",		{ RMrDX }, 0 },
> > -  { "push{!P|}",		{ RMrBX }, 0 },
> > -  { "push{!P|}",		{ RMrSP }, 0 },
> > -  { "push{!P|}",		{ RMrBP }, 0 },
> > -  { "push{!P|}",		{ RMrSI }, 0 },
> > -  { "push{!P|}",		{ RMrDI }, 0 },
> > +  { "push!P",		{ RMrAX }, 0 },
> > +  { "push!P",		{ RMrCX }, 0 },
> > +  { "push!P",		{ RMrDX }, 0 },
> > +  { "push!P",		{ RMrBX }, 0 },
> > +  { "push!P",		{ RMrSP }, 0 },
> > +  { "push!P",		{ RMrBP }, 0 },
> > +  { "push!P",		{ RMrSI }, 0 },
> > +  { "push!P",		{ RMrDI }, 0 },
> >    /* 58 */
> > -  { "pop{!P|}",		{ RMrAX }, 0 },
> > -  { "pop{!P|}",		{ RMrCX }, 0 },
> > -  { "pop{!P|}",		{ RMrDX }, 0 },
> > -  { "pop{!P|}",		{ RMrBX }, 0 },
> > -  { "pop{!P|}",		{ RMrSP }, 0 },
> > -  { "pop{!P|}",		{ RMrBP }, 0 },
> > -  { "pop{!P|}",		{ RMrSI }, 0 },
> > -  { "pop{!P|}",		{ RMrDI }, 0 },
> > +  { "pop!P",		{ RMrAX }, 0 },
> > +  { "pop!P",		{ RMrCX }, 0 },
> > +  { "pop!P",		{ RMrDX }, 0 },
> > +  { "pop!P",		{ RMrBX }, 0 },
> > +  { "pop!P",		{ RMrSP }, 0 },
> > +  { "pop!P",		{ RMrBP }, 0 },
> > +  { "pop!P",		{ RMrSI }, 0 },
> > +  { "pop!P",		{ RMrDI }, 0 },
> >    /* 60 */
> >    { X86_64_TABLE (X86_64_60) },
> >    { X86_64_TABLE (X86_64_61) },
> > @@ -10621,6 +10621,19 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  	case 'P':
> >  	  if (l == 0)
> >  	    {
> > +	      /* For pushp and popp, do not print {rex2} for them.  */
> > +	      if (ins->address_mode == mode_64bit && !cond
> 
> I don't think the mode_64bit check is needed here, as without that REX.W
> cannot possibly be set (nor can a REX2 prefix be present).

Done.

> 
> > +		  && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
> > +		{
> > +		  *ins->obufp++ = 'p';
> > +		  ins->rex2 |= 16;
> 
> Please no new use of magic constants. Have a #define with a suitable name,
> and use that here. Also I think the comment you have ahead of the if() actually
> belongs here?
> 

How about " #define IMPLICIT_REX2 16",  PUSHP/POPP can share it with JMPABS.


> > +		  break;
> > +		}
> > +
> > +	      /* If "!p" printis nothing in intel_syntax.  */
> > +	      if (!cond && ins->intel_syntax)
> > +		break;
> 
> The comment isn't quite right ('p' is printed). Also (nit) "prints".
> 

Added "!cond"  here, since just removed {} for !p,  I think P has its way of handling intel_syntax.

> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> > @@ -225,6 +225,7 @@ push, 0x68, i186&No64,
> > DefaultSize|No_bSuf|No_sSuf|No_qSuf, { Imm16|Imm32 }  push, 0x6,
> No64,
> > DefaultSize|No_bSuf|No_sSuf|No_qSuf, { SReg }  // In 64bit mode, the
> operand size is implicitly 64bit.
> >  push, 0x50, x64, No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64 }
> > +pushp, 0x50, APX_F, No_bSuf|No_lSuf|No_sSuf|Rex2, {Reg64 }
> 
> Since Reg16 isn't allowed, you also want No_wSuf here (and below). Note also
> the missing blank after the opening figure brace.
> 

Done.

> The new Rex2 attribute is not only wasteful (it can easily be a new enumerator
> used with OperandConstraint), but also misleading. We don't just need REX2
> here, but we need it with REX2.W set. Even if from the tc-i386.c changes it
> looks as if that was happening implicitly (presumably due to the absence of
> NoRex64), naming still needs to properly reflect the purpose.
> 

You are right, rex2.w is set in process_suffix, since there is no NoRex64. 

How about Rex2W? 
if (i.tm.opcode_modifier.rex2w)
{
    i.rex2_encoding = true;
    i.rex |= REX_W;   // add NoRex64 back, and set REX_W here.
}


Or just add a special judgment?

if (t->mnem_off == MN_pushp || t->mnem_off == MN_popp)
{
    i.rex2_encoding = true;
    i.rex |= REX_W;   // add NoRex64 back, and set REX_W here.
}

By the way, Is our APX patches (V3)  review still in progress? Since we have to commit them into the trunk before the next release, time is a bit tight ( many people may have vacation plans at the end of the year).

Thanks,
Lili.