[PATCH] Support APX PUSHP/POPP

[Cui, Lili]

> >>> --- 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.
> 
> But what's implicit about the REX2 prefix here?
> 

PUSHP/POPP requires rex2.w ==1 and we don't want to print {rex2} for it. JMPABS has the same need, so I want to define a common macro for them, or NO_NEED_PRINT_REX2?

> >>> +		  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.
> 
> All fine. Yet still the comment should reflect reality.
> 

Changed.

> >>> --- 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.
> > }
> 
> I'd prefer the latter over a new attribute (albeit once again with the comment
> actually matching code), and perhaps I view the latter equal to my earlier
> suggestion.
> 

Ok, put them in process_operands, there's already a special handler here to change the prefix.

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 0dde2a9ad44..2f2b1b04d10 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -8715,6 +8715,13 @@ process_operands (void)
       i.tm.operands++;
     }

+  /* PUSHP/POPP requires rex2.w == 1.  */
+  if (i.tm.mnem_off == MN_pushp || i.tm.mnem_off == MN_popp)
+    {
+      i.rex2_encoding = true;
+      i.rex |= REX_W;
+    }
+
   if (i.tm.opcode_modifier.sse2avx && i.tm.opcode_modifier.vexvvvv)
     {

> > 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).
> 
> Hmm, not exactly sure what to say. You posted v3 only on Friday. Plus I have
> spent an enormous amount of time reviewing v1 and v2. Please be patient, I
> have this queued for another round of review. And no, there is no "we have
> to ... before the next release". You (Intel) may want to, but with a large new
> feature like this it had to be clear that getting all pieces in will take time.
> 

Maybe what I said is inappropriate and makes you uncomfortable, so just follow your own pace. It's not easy to review the code of such a large function, and I really know it will take up a lot of your time, at least I can't do such a detailed inspection as you do.

Thanks,
Lili.