RE: [PATCH v3 7/9] Support APX Push2/Pop2

["Cui, Lili" <lili.cui@intel.com>]

> On 24.11.2023 08:02, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -248,6 +248,7 @@ enum i386_error
> >      invalid_vector_register_set,
> >      invalid_tmm_register_set,
> >      invalid_dest_and_src_register_set,
> > +    invalid_src_register_set,
> >      invalid_pseudo_prefix,
> >      unsupported_vector_index_register,
> >      unsupported_broadcast,
> > @@ -256,6 +257,7 @@ enum i386_error
> >      mask_not_on_destination,
> >      no_default_mask,
> >      unsupported_rc_sae,
> > +    unsupported_rsp_register,
> >      invalid_register_operand,
> >      internal_error,
> >    };
> > @@ -5398,6 +5400,9 @@ md_assemble (char *line)
> >  	case invalid_dest_and_src_register_set:
> >  	  err_msg = _("destination and source registers must be distinct");
> >  	  break;
> > +	case invalid_src_register_set:
> 
> Did you mean invalid_dest_register_set and ...
> 
> > +	  err_msg = _("two source registers must be distinct");
> 
> ... "two destination ..."? This is for POP2, after all, which has no source register
> at all.
> 

Done.

> > @@ -5422,6 +5427,9 @@ md_assemble (char *line)
> >  	case unsupported_rc_sae:
> >  	  err_msg = _("unsupported static rounding/sae");
> >  	  break;
> > +	case unsupported_rsp_register:
> > +	  err_msg = _("cannot be used with %rsp register");
> > +	  break;
> 
> While this wording looks okay as visible here, please consider it in the context
> it is used in: "cannot be used with %rsp register for `push2'"
> is, I'm sorry to say that, clumsy at best. If you want to stick to setting err_msg,
> how about "%rsp register cannot be used"? Personally I'd prefer a resulting
> output of "%rsp register cannot be used with `push2'", but I wouldn't insist on
> you going that route if you don't like that.
> 

"%rsp register cannot be used" ,this is much better, thanks.

> > @@ -7113,6 +7121,33 @@ check_EgprOperands (const insn_template *t)
> >    return 0;
> >  }
> >
> > +/* Check if APX operands are valid for the instruction.  */ static
> > +int
> 
> Please can functions returning boolean indicators have a return type of "bool"
> (and perhaps use "true" as the success indicator, not "false")?
> 

Done.

> > +check_APX_operands (const insn_template *t) {
> > +  /* Push2* and Pop2* cannot use RSP and Pop2* cannot pop two same
> registers.
> > +   */
> > +  if (t->mnem_off == MN_push2 || t->mnem_off == MN_push2p
> > +      || t->mnem_off == MN_pop2 || t->mnem_off == MN_pop2p)
> 
> Considering (perhaps just theoretical) further additions here, did you consider
> using switch()? Even without further additions this would imo be more legible
> (due to there being slightly less redundancy).
> 

Done.

  /* Push2* and Pop2* cannot use RSP and Pop2* cannot pop two same registers.
   */
  switch (t->mnem_off)
    {
    case MN_pop2:
    case MN_pop2p:
      if (register_number (i.op[0].regs) == register_number (i.op[1].regs))
        {
          i.error = invalid_dest_register_set;
          return 1;
        }
    case MN_push2:
    case MN_push2p:
      if (register_number (i.op[0].regs) == 4
          || register_number (i.op[1].regs) == 4)
        {
          i.error = unsupported_rsp_register;
          return 1;
        }
    }

> > --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> > @@ -28,3 +28,9 @@ _start:
> >  	.byte 0xff
> >  	#{evex} inc %rax %rbx EVEX.vvvv' != 1111 && EVEX.ND = 0.
> >  	.insn EVEX.L0.NP.M4.W1 0xff, %rax, %rbx
> > +	.byte 0xff
> > +	# pop2 %rax, %rbx set EVEX.ND=0.
> > +	.byte 0x62,0xf4,0x64,0x08,0x8f,0xc0
> > +	.byte 0xff, 0xff, 0xff
> > +	# pop2 %rax set EVEX.vvvv' = 1111.
> 
> Another instance of the unclear EVEX.vvvv' (i.e. the questionable nature if '
> here). Yet then - what is the test below checking? EVEX.vvvv encodes one of
> the two operands, so all values are valid? Isn't this about both operands being
> the same? That would better be said then explicitly, e.g.
> simply
> 
> 	# pop2 %rax, %rax (twice same destination)
> 
> > +	.byte 0x62,0xf4,0x7c,0x18,0x8f,0xc0
> 
> Also again both new tests use .byte instead of .insn: Is there a particular
> reason? Here are a couple of examples that I have readily available (Intel
> syntax again, ftaod):
> 
> 	.insn EVEX.L0.M4.W0 0x8f/0, r8, rax{sae}	; pop2 r8, rax
> 	.insn EVEX.L0.M4.W0 0x8f/0, xmm16, rax{sae}	; pop2 r16, rax
> 	.insn EVEX.L0.M4.W0 0x8f/0, rax, r8{sae}	; pop2 rax, r8
> 	.insn EVEX.L0.M12.W0 0x8f/0, rax, rax{sae}	; pop2 rax, r16
> 	.insn EVEX.L0.M4.W1 0x8f/0, rax, rcx{sae}	; pop2.x rax, rcx
> 
> I'm sure you can derive from them what you're actually after.
> 

Thanks!

        # pop2 %rax, %r8 set EVEX.ND=0.
        .insn EVEX.L0.M4.W0 0x8f/0,  %rax, %r8
        .byte 0xff, 0xff, 0xff
        # pop2 %rax, %r8 set EVEX.vvvv = 1111.
        .insn EVEX.L0.M4.W0 0x8f,  %rax, {rn-sae},%r8
        # pop2 %r8, %r8.
        .insn EVEX.L0.M4.W0 0x8f/0,  %r8,{rn-sae}, %r8


> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-push2pop2.s
> > @@ -0,0 +1,39 @@
> > +# Check 64bit APX-Push2Pop2 instructions
> > +
> > +	.allow_index_reg
> > +	.text
> > +_start:
> > +	push2 %rbx, %rax
> > +	push2 %r17, %r8
> > +	push2 %r9, %r31
> > +	push2 %r31, %r24
> > +	push2p %rbx, %rax
> > +	push2p %r17, %r8
> > +	push2p %r9, %r31
> > +	push2p %r31, %r24
> > +	pop2 %rax, %rbx
> > +	pop2 %r8, %r17
> > +	pop2 %r31, %r9
> > +	pop2 %r24, %r31
> > +	pop2p %rax, %rbx
> > +	pop2p %r8, %r17
> > +	pop2p %r31, %r9
> > +	pop2p %r24, %r31
> > +
> > +.intel_syntax noprefix
> 
> Nit: Un-indented directive again.

Done.

> 
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -105,6 +105,7 @@ static bool FXSAVE_Fixup (instr_info *, int, int);
> > static bool MOVSXD_Fixup (instr_info *, int, int);  static bool
> > DistinctDest_Fixup (instr_info *, int, int);  static bool
> > PREFETCHI_Fixup (instr_info *, int, int);
> > +static bool PUSH2_POP2_Fixup (instr_info *, int, int);
> >
> >  static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
> >  						enum disassembler_style,
> > @@ -225,6 +226,9 @@ struct instr_info
> >    }
> >    vex;
> >
> > +/* For APX EVEX-promoted prefix, EVEX.ND shares the same bit as
> > +vex.b.  */ #define nd b
> 
> Can this be moved ahead to patch 4, such that it can be used there (instead of
> vex.b) as well? IOW ...
> 
> > @@ -9125,7 +9133,7 @@ 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->evex_type == evex_from_legacy && !ins->vex.b
> > +      if (ins->evex_type == evex_from_legacy && !ins->vex.nd
> >  	  && (ins->vex.register_specifier || !ins->vex.v))
> >  	return &bad_opcode;
> 
> ... neither this nor ...
> 
> > @@ -13388,11 +13396,10 @@ OP_VEX (instr_info *ins, int bytemode, int
> sizeflag ATTRIBUTE_UNUSED)
> >    if (!ins->need_vex)
> >      return true;
> >
> > -  /* Here vex.b is treated as "EVEX.ND".  */
> >    if (ins->evex_type == evex_from_legacy)
> >      {
> >        ins->evex_used |= EVEX_b_used;
> > -      if (!ins->vex.b)
> > +      if (!ins->vex.nd)
> >  	return true;
> >      }       
> 
> ... this should require touching here.
> 

I moved them into NDD patch, , which adds these checks.

> > @@ -13884,3 +13894,26 @@ 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) {
> > +  if (ins->modrm.mod != 3 || !ins->vex.b)
> 
> Did you mean vex.nd? Plus, considering the vex.nd check further down, why is
> this checked both here and there?
> 

Dropped.

> > +    return true;
> 
> Doesn't this result in silently bogus/wrong output? Shouldn't you print
> "(bad)" like you do further down? At which point it may make sense to simply
> fold both if()s?
> 
> > --- a/opcodes/i386-opc.h
> > +++ b/opcodes/i386-opc.h
> > @@ -807,6 +807,7 @@ typedef struct i386_opcode_modifier
> >    unsigned int isa64:2;
> >    unsigned int noegpr:1;
> >    unsigned int nf:1;
> > +  unsigned int push2pop2:1;
> >  } i386_opcode_modifier;
> 
> Still a new modifier despite my earlier request to avoid adding one when you
> easily can? Here OperandConstraint is actually fully applicable to use, as what
> you want to enforce is a constraint on operands.
> 

I also found this issue and removed it locally.

Thanks,
Lili.