Re: [PATCH 6/8] Support APX Push2/Pop2

[Jan Beulich <jbeulich@suse.com>]

On 02.11.2023 12:29, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -256,6 +256,7 @@ enum i386_error
>      mask_not_on_destination,
>      no_default_mask,
>      unsupported_rc_sae,
> +    unsupported_rsp_register,
>      invalid_register_operand,
>      internal_error,
>    };
> @@ -5476,6 +5477,9 @@ md_assemble (char *line)
>  	case unsupported_rc_sae:
>  	  err_msg = _("unsupported static rounding/sae");
>  	  break;
> +	case unsupported_rsp_register:
> +	  err_msg = _("unsupported rsp register");
> +	  break;

Perhaps you mean "cannot be used with" or some such? Also the register
name needs conditionally prefixing with % in diagnostics.

> @@ -6854,6 +6858,24 @@ check_VecOperands (const insn_template *t)
>  	}
>      }
>  
> +  /* Push2/Pop2 cannot use RSP and Pop2 cannot pop two same registers.  */
> +  if (t->opcode_modifier.push2pop2)

I question this way of recognizing these two insns: You introduce a
whole new table column here just to have two entries set this bit.
This is cheaper by comparing the mnemonic offsets, as we do elsewhere
in various cases.

I also disagree with putting the check in check_VecOperands():
There's nothing vector-ish here. Either you put it straight in the
caller, or you introduce a new check_APX_operands().

> +    {
> +      unsigned int reg1 = register_number (i.op[0].regs);
> +      unsigned int reg2 = register_number (i.op[1].regs);
> +
> +      if (reg1 == 0x4 || reg2 == 0x4)
> +	{
> +	  i.error = unsupported_rsp_register;
> +	  return 1;
> +	}
> +      if (t->base_opcode == 0x8f && reg1 == reg2)
> +	{
> +	  i.error = invalid_dest_and_src_register_set;

This enumerator's disagnostic talks about source and destination
register, which isn't applicable here.

> --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> @@ -30,3 +30,9 @@ _start:
>          .byte 0xff
>          #{evex} inc %rax EVEX.vvvv' > 0 (illegal value).
>          .byte 0x62, 0xf4, 0xec, 0x08, 0xff, 0xc0
> +        .byte 0xff, 0xff
> +	# pop2 %rax, %rbx set EVEX.ND=0.
> +        .byte 0x62,0xf4,0x64,0x08,0x8f,0xc0
> +        .byte 0xff, 0xff, 0xff
> +	# pop2 %rax, %rsp set EVEX.VVVV=0xf.
> +        .byte 0x62,0xf4,0x7c,0x18,0x8f,0xc0

This 2nd comment looks bogus. What is it that's being tested here?

Also again note indentation inconsistencies.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-push2pop2-inval.s
> @@ -0,0 +1,15 @@
> +# Check illegal APX-Push2Pop2 instructions
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +	push2  %eax, %ebx

It's okay to test 32-bit operands, but more important is to test
16-bit ones, as only those could (also) be used with PUSH/POP.

> --- a/opcodes/i386-dis-evex-mod.h
> +++ b/opcodes/i386-dis-evex-mod.h
> @@ -1,4 +1,9 @@
>  /* Nothing at present.  */
> +  /* MOD_EVEX_MAP4_8F_R_0 */
> +  {
> +    { Bad_Opcode },
> +    { PREFIX_TABLE (PREFIX_EVEX_MAP4_8F_R_0_M_1) },
> +  },
>    /* MOD_EVEX_MAP4_DA_PREFIX_1 */
>    {
>      { Bad_Opcode },
> @@ -41,3 +46,8 @@
>    {
>      { "movdiri",	{ Edq, Gdq }, 0 },
>    },
> +  /* MOD_EVEX_MAP4_FF_R_6 */
> +  {
> +    { Bad_Opcode },
> +    { PREFIX_TABLE (PREFIX_EVEX_MAP4_FF_R_6_M_1) },
> +  },

Same comment as before regarding additions to this file.

> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
>[...]
> @@ -9011,6 +9020,8 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	case 0x4:
>  	  vex_table_index = EVEX_MAP4;
>  	  ins->evex_type = evex_from_legacy;
> +	  if (ins->address_mode != mode_64bit)
> +	    return &bad_opcode;
>  	  break;

This looks to belong into an earlier patch.

> @@ -9073,8 +9084,9 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	{
>  	  /* EVEX from legacy instructions, when the EVEX.ND bit is 0,
>  	     all bits of EVEX.vvvv and EVEX.V' must be 1.  */
> -	  if (!ins->vex.b && (ins->vex.register_specifier
> -				  || !ins->vex.v))
> +	  if (ins->vex.ll || (!ins->vex.b
> +			      && (ins->vex.register_specifier
> +				  || !ins->vex.v)))
>  	    return &bad_opcode;

This as well.

> @@ -13821,3 +13836,24 @@ PREFETCHI_Fixup (instr_info *ins, int bytemode, int sizeflag)
>  
>    return OP_M (ins, bytemode, sizeflag);
>  }
> +
> +static bool
> +PUSH2_POP2_Fixup (instr_info *ins, int bytemode, int sizeflag)
> +{
> +  unsigned int vvvv_reg = ins->vex.register_specifier
> +    | !ins->vex.v << 4;

Nit: Please parenthesize the shift.

> +  unsigned int rm_reg = ins->modrm.rm + (ins->rex & REX_B ? 8 : 0)
> +    + (ins->rex2 & REX_B ? 16 : 0);
> +
> +  /* Here vex.b is treated as "EVEX.ND.  */
> +  /* Push2/Pop2 cannot use RSP and Pop2 cannot pop two same registers.  */

The two comments want folding. As to the former, though: How about having

#define nd b

in the EVEX struct declaration (provided we don't have any variables named
"nd" right now), ...

> +  if (!ins->vex.b || vvvv_reg == 0x4 || rm_reg == 0x4

... allowing to use ins->vex.nd here (at which point that comment is
unnecessary)?

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3494,3 +3494,10 @@ erets, 0xf20f01ca, FRED|x64, NoSuf, {}
>  eretu, 0xf30f01ca, FRED|x64, NoSuf, {}
>  
>  // FRED instructions end.
> +
> +// APX Push2/Pop2 instruction.
> +
> +push2, 0xff/6, APX_F, Modrm|VexW0|EVex128|Push2Pop2|EVexMap4|VexVVVVSrc|No_bSuf|No_wSuf|No_lSuf|No_sSuf, { Reg64, Reg64 }
> +push2p, 0xff/6, APX_F, Modrm|VexW1|EVex128|Push2Pop2|EVexMap4|VexVVVVSrc|No_bSuf|No_wSuf|No_lSuf|No_sSuf, { Reg64, Reg64 }
> +pop2, 0x8f/0, APX_F, Modrm|VexW0|EVex128|Push2Pop2|EVexMap4|VexVVVVSrc|No_bSuf|No_wSuf|No_lSuf|No_sSuf, { Reg64, Reg64 }
> +pop2p, 0x8f/0, APX_F, Modrm|VexW1|EVex128|Push2Pop2|EVexMap4|VexVVVVSrc|No_bSuf|No_wSuf|No_lSuf|No_sSuf, { Reg64, Reg64 }

Like other extensions have it, there also wants to be an "end" comment.

Jan