[PATCH 6/8] Support APX Push2/Pop2

[Cui, Lili]

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

Done.

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

Done.

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

How about  putting check_EgprOperands into check_APX_operands ?

      /* Check if EGRPS operands(r16-r31) are valid.  */
      if (check_EgprOperands (t))
        {
          specific_error = progress (i.error);
          continue;
        }

      /* Check if APX operands are valid.  */
      if (check_APX_operands (t))
        {
          specific_error = progress (i.error);
          continue;
        }

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

Done.

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

I think it should be  # pop2 %rax set EVEX.vvvv' = 1111. It wants to test that pop2 has only one operand when decoding.

> Also again note indentation inconsistencies.
> 

Done.

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

Done.

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

Done.

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

Done.

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

Deleted, redundant.

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

Done.

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

Done.

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

Done.

Thanks,
Lili.