[PATCH] Support APX PUSHP/POPP

[Jan Beulich]

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

> +		  && 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?

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

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

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.

Jan