RE: [PATCH v4 1/9] Support APX GPR32 with rex2 prefix

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

> On 19.12.2023 13:12, Cui, Lili wrote:
> > @@ -5283,6 +5321,9 @@ md_assemble (char *line)
> >  	case unsupported_syntax:
> >  	  err_msg = _("unsupported syntax");
> >  	  break;
> > +	case unsupported_EGPR_for_addressing:
> > +	  err_msg = _("extended GPR cannot be used as base/index");
> > +	  break;
> 
> While this one's now suitable for the as_bad() below the switch, ...
> 
> > @@ -5336,6 +5377,9 @@ md_assemble (char *line)
> >  	case invalid_dest_and_src_register_set:
> >  	  err_msg = _("destination and source registers must be distinct");
> >  	  break;
> > +	case invalid_pseudo_prefix:
> > +	  err_msg = _("rex2 pseudo prefix cannot be used here");
> > +	  break;
> 
> ... this one still doesn't really fit the "... for `<insn>'" there. At least the "here"
> needs dropping.
> 
Changed to :

"rex2 pseudo prefix cannot be used"

> > @@ -5630,11 +5681,12 @@ md_assemble (char *line)
> >  	  && (i.op[1].regs->reg_flags & RegRex64) != 0)
> >        || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> >  	   || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > -	  && i.rex != 0))
> > +	  && (i.rex != 0 || i.rex2 != 0)))
> >      {
> >        int x;
> >
> > -      i.rex |= REX_OPCODE;
> > +      if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
> > +	i.rex |= REX_OPCODE;
> >        for (x = 0; x < 2; x++)
> >  	{
> >  	  /* Look for 8 bit operand that uses old registers.  */ @@ -5645,7
> > +5697,7 @@ md_assemble (char *line)
> >  	      /* In case it is "hi" register, give up.  */
> >  	      if (i.op[x].regs->reg_num > 3)
> >  		as_bad (_("can't encode register '%s%s' in an "
> > -			  "instruction requiring REX prefix."),
> > +			  "instruction requiring REX/REX2 prefix."),
> >  			register_prefix, i.op[x].regs->reg_name);
> >
> >  	      /* Otherwise it is equivalent to the extended register.
> > @@ -5657,11 +5709,11 @@ md_assemble (char *line)
> >  	}
> >      }
> >
> > -  if (i.rex == 0 && i.rex_encoding)
> > +  if (i.rex == 0 && i.rex2 == 0 && (i.rex_encoding ||
> > + i.rex2_encoding))
> >      {
> >        /* Check if we can add a REX_OPCODE byte.  Look for 8 bit operand
> >  	 that uses legacy register.  If it is "hi" register, don't add
> > -	 the REX_OPCODE byte.  */
> > +	 rex and rex2 prefix.  */
> >        int x;
> >        for (x = 0; x < 2; x++)
> >  	if (i.types[x].bitfield.class == Reg @@ -5671,6 +5723,7 @@
> > md_assemble (char *line)
> >  	  {
> >  	    gas_assert (!(i.op[x].regs->reg_flags & RegRex));
> >  	    i.rex_encoding = false;
> > +	    i.rex2_encoding = false;
> >  	    break;
> >  	  }
> >
> > @@ -5678,7 +5731,13 @@ md_assemble (char *line)
> >  	i.rex = REX_OPCODE;
> >      }
> >
> > -  if (i.rex != 0)
> > +  if (is_apx_rex2_encoding ())
> > +    {
> > +      build_rex2_prefix ();
> > +      /* The individual REX.RXBW bits got consumed.  */
> > +      i.rex &= REX_OPCODE;
> > +    }
> > +  else if (i.rex != 0)
> >      add_prefix (REX_OPCODE | i.rex);
> >
> >    insert_lfence_before (last_insn);
> 
> All of this will need re-basing over "x86: properly respect rex/{rex}", with the
> result (I hope) that .insn will then also be covered REX2-wise.
> 
Done.

> > @@ -5752,6 +5811,20 @@ parse_insn (const char *line, char *mnemonic,
> bool prefix_only)
> >  	    goto too_long;
> >  	  *mnem_p = '\0';
> >
> > +	  /* Point l at the closing brace if there's no other separator.  */
> > +	  if (*l != END_OF_INSN && !is_space_char (*l)
> > +	      && *l != PREFIX_SEPARATOR)
> > +	    --l;
> > +	}
> > +      /* Skip the immediate 0x** of {rex2 0x00} prefix.  */
> > +      else if (*mnemonic == '{'&& is_space_char (*l))
> 
> Nit: Missing blank.
> 
> > +	{
> > +	  while ( *l != '}')
> 
> Nit: Stray blank.
> 
> > +	    ++l;
> 
> What if there's no '}' on the line?
> 
> > +	  *mnem_p++ = *l++;
> > +	  if (mnem_p >= mnemonic + MAX_MNEM_SIZE)
> > +	    goto too_long;
> > +	  *mnem_p = '\0';
> 
> You skip everything, not just 0xNN. You also skip stuff after other pseudo
> prefixes, if I'm not mistaken. That's both too lax. However, skipping isn't an
> option here anyway. Either we actually respect what the user has written, or
> we error out. Skipping therefore is an option only if the provided expression
> (not just plain number!) evaluates to 0. I don't understand anyway why this
> code was added: When I asked about the specific plans, H.J. clearly said the
> form with a constant would be a disassembler- only thing for now.
> 

It should be only supported in disassembler, I will drop it in assembler.

> > @@ -7005,6 +7082,43 @@ VEX_check_encoding (const insn_template *t)
> >    return 0;
> >  }
> >
> > +/* Check if Egprs operands are valid for the instruction.  */
> > +
> > +static int
> > +check_EgprOperands (const insn_template *t)
> 
> Hmm, I thought I had asked before to make functions with boolean return
> values have a return type of bool, and then use "true" for success. An
> alternative would be to return the error indicator, rather than putting it in
> i.error here.
> 
> Then again I realize this is in line with VEX_check_encoding() and
> check_VecOperands() (which I think would better be changed, but anyway).
> 

Changed it to bool. For the rest, it's a bit strange to only change check_EgprOperands. Can this place be left unchanged? Or should I submit a new patch and change the old one first?

> > @@ -7149,6 +7263,14 @@ match_template (char mnem_suffix)
> >  	      continue;
> >  	    }
> >
> > +	  /* Check if pseudo prefix {rex2} is valid.  */
> > +	  if (t->opcode_modifier.noegpr && i.rex2_encoding)
> > +	    {
> > +	      i.error = invalid_pseudo_prefix;
> 
> What is this needed for? I.e. why can't you pass the value ...
> 
> > +	      specific_error = progress (i.error);
> 
> ... in here directly (as is done elsewhere as well)?
> 

Done.

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-rex2.s
> > @@ -0,0 +1,86 @@
> > +## REX2.B3 bit
> > +	 sub   (%r10), %r31
> > +	 sub   (%r13), %r31
> 
> Nit: There's still an indentation anomaly here.
> 
Done.

> > --- a/gas/testsuite/gas/i386/x86-64-inval-pseudo.s
> > +++ b/gas/testsuite/gas/i386/x86-64-inval-pseudo.s
> > @@ -1,4 +1,8 @@
> >  	.text
> >  	{disp16} movb (%ebp),%al
> >  	{disp16} movb (%rbp),%al
> > +
> > +	/* Instruction not support APX.  */
> > +	{rex2} xsave (%r15, %rbx)
> > +	{rex2} xsave64 (%r15, %rbx)
> >  	.p2align 4,0
> 
> Aren't these dealt with (in a more complete fashion) in x86-64-pseudos-
> bad.s?
> 
Removed.

> > --- a/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > +++ b/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > @@ -5,3 +5,77 @@ pseudos:
> >  	{rex} vmovaps %xmm7,%xmm2
> >  	{rex} vmovaps %xmm17,%xmm2
> >  	{rex} rorx $7,%eax,%ebx
> > +	{rex2} vmovaps %xmm7,%xmm2
> > +	{rex2} xsave (%rax)
> > +	{rex2} xsaves (%ecx)
> > +	{rex2} xsaves64 (%ecx)
> > +	{rex2} xsavec (%ecx)
> > +	{rex2} xrstors (%ecx)
> > +	{rex2} xrstors64 (%ecx)
> 
> Here.
> 
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -144,6 +144,12 @@ struct instr_info
> >    /* Bits of REX we've already used.  */
> >    uint8_t rex_used;
> >
> > +  /* Record W R4 X4 B4 bits for rex2.  */  unsigned char rex2;
> > +  /* Bits of REX2 we've already used.  */  unsigned char rex2_used;
> 
> When you say REX2, one ought to be permitted to imply you mean the prefix,
> not the struct field. That's ambiguous here, though - bit positions used match
> those in rex2, not those in the REX2 payload.
> IOW either you use lower case to make more obvious that you mean the other
> struct field, or you say e.g. "REX2 prefix bits we've already used." Albeit that
> would still be imprecise, as other REX2 prefix bits' use is recorded in rex_used.
> 

Changed to 

  /* Bits of rex2 we've already used.  */
  unsigned char rex2_used;

> > @@ -265,8 +272,13 @@ struct dis_private {
> >    {							\
> >      if (value)						\
> >        {							\
> > -	if ((ins->rex & value))				\
> > +	if (ins->rex & value)				\
> >  	  ins->rex_used |= (value) | REX_OPCODE;	\
> 
> Like is done here, ...
> 
> > +	if (ins->rex2 & value)				\
> > +	  {						\
> > +	    ins->rex2_used |= value;			\
> 
> other uses of "value" also want parenthesizing, unless not used with any kind
> of operator (e.g. in the if() above).
> 
Done.

> > @@ -276,6 +288,9 @@ struct dis_private {  #define EVEX_b_used 1
> > #define EVEX_len_used 2
> >
> > +/* M0 in rex2 prefix represents map0 or map1.  */ #define REX2_M 0x8
> 
> Extending an earlier comment: This really should go next to REX_W and
> friends. In principle the assembler could want to use this constant as well,
> hence why it would better go the opcode/i386.h anyway.
> 
Done.

> > @@ -4196,19 +4221,19 @@ static const struct dis386 x86_64_table[][2] =
> > {
> >
> >    /* X86_64_E8 */
> >    {
> > -    { "callP",		{ Jv, BND }, 0 },
> > -    { "call@",		{ Jv, BND }, 0 }
> > +    { "callP",		{ Jv, BND }, PREFIX_REX2_ILLEGAL },
> 
> This, ...
> 
> > +    { "call@",		{ Jv, BND }, PREFIX_REX2_ILLEGAL }
> >    },
> >
> >    /* X86_64_E9 */
> >    {
> > -    { "jmpP",		{ Jv, BND }, 0 },
> > -    { "jmp@",		{ Jv, BND }, 0 }
> > +    { "jmpP",		{ Jv, BND }, PREFIX_REX2_ILLEGAL },
> 
> ... this, and ...
> 
> > +    { "jmp@",		{ Jv, BND }, PREFIX_REX2_ILLEGAL }
> >    },
> >
> >    /* X86_64_EA */
> >    {
> > -    { "{l|}jmp{P|}", { Ap }, 0 },
> > +    { "{l|}jmp{P|}", { Ap }, PREFIX_REX2_ILLEGAL },
> >    },
> 
> ... this change isn't really needed, is it? The marker is only needed on 64-bit
> insns (i.e. respective slots 2 of x86_64_table[] entries).
> 
Done.

Thanks,
Lili.