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

[Jan Beulich <jbeulich@suse.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.

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

> @@ -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")?

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

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

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

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

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

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

Jan