[PATCH] Support APX PUSHP/POPP

[Jan Beulich]

On 27.11.2023 14:45, Cui, Lili wrote:
>> 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.

But what's implicit about the REX2 prefix here?

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

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

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

Jan