Re: [PATCH 4/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

On 19.09.2023 17:25, Cui, Lili wrote:
> From: konglin1 <lingling.kong@intel.com>
> 
> opcodes/ChangeLog:
> 
> 	* opcodes/i386-dis-evex-prefix.h: Add NDD decode for adox/adcx.
> 	* opcodes/i386-dis-evex-reg.h: Handle for REG_EVEX_MAP4_80,
> 	REG_EVEX_MAP4_81, REG_EVEX_MAP4_83, REG_EVEX_MAP4_C0,
> 	REG_EVEX_MAP4_C1, REG_EVEX_MAP4_D0, REG_EVEX_MAP4_D1,
> 	REG_EVEX_MAP4_D2, REG_EVEX_MAP4_D3, REG_EVEX_MAP4_F6,
> 	REG_EVEX_MAP4_F7, REG_EVEX_MAP4_FE, REG_EVEX_MAP4_FF.
> 	* opcodes/i386-dis-evex.h: Add NDD insn.
> 	* opcodes/i386-dis.c (VexGb): Add new define.
> 	(VexGv): Ditto.
> 	(get_valid_dis386): Change for NDD decode.
> 	(print_insn): Ditto.
> 	(print_register): Ditto.
> 	(intel_operand_size): Ditto.
> 	(OP_E_memory): Ditto.
> 	(OP_VEX): Ditto.
> 	* opcodes/i386-opc.h (Opcode_APX_NDDD): New macro.
> 	* opcodes/i386-opc.tbl: Add APX NDD instructions.
> 	* opcodes/i386-tbl.h: Regenerated.
> 
> gas/ChangeLog:
> 
> 	* gas/config/tc-i386.c (is_any_apx_encoding): Add legacy insn
> 	promote to SPACE_EVEXMAP4.
> 	(build_legacy_insns_with_apx_encoding): Add ndd bit encode.
> 	(md_assemble): Change for ndd encode.
> 	(process_operands): Ditto.
> 	(build_modrm_byte): Ditto.
> 	(operand_size_match):
> 	Support APX NDD that the number of operands is 3.
> 	(match_template): Support swap the first two operands for
> 	APX NDD that the number of operands is 3.
> 	* gas/testsuite/gas/i386/x86-64.exp: Add x86-64-apx-ndd.
> 	* gas/testsuite/gas/i386/x86-64-apx-ndd.d: New test.
> 	* gas/testsuite/gas/i386/x86-64-apx-ndd.s: Ditto.
> 	* testsuite/gas/i386/x86-64-pseudos.d: Add test.
> 	* testsuite/gas/i386/x86-64-pseudos.s: Ditto.
> ---
>  gas/config/tc-i386.c                    |  80 ++++++++----
>  gas/testsuite/gas/i386/x86-64-apx-ndd.d | 165 ++++++++++++++++++++++++
>  gas/testsuite/gas/i386/x86-64-apx-ndd.s | 156 ++++++++++++++++++++++
>  gas/testsuite/gas/i386/x86-64-pseudos.d |  42 ++++++
>  gas/testsuite/gas/i386/x86-64-pseudos.s |  43 ++++++
>  gas/testsuite/gas/i386/x86-64.exp       |   1 +
>  opcodes/i386-dis-evex-prefix.h          |   4 +-
>  opcodes/i386-dis-evex-reg.h             | 123 ++++++++++++++++++
>  opcodes/i386-dis-evex.h                 | 124 +++++++++---------
>  opcodes/i386-dis.c                      |  47 ++++++-
>  opcodes/i386-opc.h                      |   1 +
>  opcodes/i386-opc.tbl                    |  67 ++++++++++
>  12 files changed, 762 insertions(+), 91 deletions(-)
>  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-ndd.d
>  create mode 100644 gas/testsuite/gas/i386/x86-64-apx-ndd.s
> 
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index 48916bc3846..381e389bb04 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2261,8 +2261,9 @@ operand_size_match (const insn_template *t)
>        unsigned int given = i.operands - j - 1;
>  
>        /* For FMA4 and XOP insns VEX.W controls just the first two
> -	 register operands.  */
> -      if (is_cpu (t, CpuFMA4) || is_cpu (t, CpuXOP))
> +	 register operands. And APX insns just swap the first operands.  */
> +      if (is_cpu (t, CpuFMA4) || is_cpu (t, CpuXOP)
> +	  || (is_cpu (t,CpuAPX_F) && i.operands == 3))
>  	given = j < 2 ? 1 - j : j;

In the comment, how about "And APX_F insns just swap the two source operands,
with the 3rd one being the destination"?

Is the "i.operands == 3" part of the condition really needed? I.e. are there
any APX_F insns which can make it here but must not take this path? Afaict
2-operand insns are fine to go here, and more-than-3-operand insns don't
come with the D attribute.

Also (nit) there's a missing blank after the comma.

> @@ -3876,6 +3877,7 @@ is_any_apx_encoding (void)
>  {
>    return i.rex2 
>      || i.rex2_encoding
> +    || i.tm.opcode_space == SPACE_EVEXMAP4
>      || (i.vex.register_specifier
>  	&& i.vex.register_specifier->reg_flags & RegRex2);
>  }
> @@ -4204,6 +4206,10 @@ build_legacy_insns_with_apx_encoding (void)
>      }
>  
>    build_evex_insns_with_extend_evex_prefix ();
> +
> +  /* Encode the NDD bit.  */
> +  if (i.vex.register_specifier)
> +    i.vex.bytes[3] |= 0x10;
>  }
>  
>  static void
> @@ -7383,26 +7389,31 @@ match_template (char mnem_suffix)
>  	  overlap1 = operand_type_and (operand_types[0], operand_types[1]);
>  	  if (t->opcode_modifier.d && i.reg_operands == i.operands
>  	      && !operand_type_all_zero (&overlap1))
> -	    switch (i.dir_encoding)
> -	      {
> -	      case dir_encoding_load:
> -		if (operand_type_check (operand_types[i.operands - 1], anymem)
> -		    || t->opcode_modifier.regmem)
> -		  goto check_reverse;
> -		break;
> +	    {
> +	      int isMemOperand = (t->opcode_modifier.vexvvvv
> +				  && t->opcode_space == SPACE_EVEXMAP4)
> +				  ? i.operands - 2 : i.operands - 1;

"is" in the variable name is properly misleading. What you're determining
here is which operand you want to _check_ for being the memory operand.

As to the condition, the two side of && may want swapping: In such a
condition it is generally desirable to have the more restricting part
first. Plus this may be more neat to express without ?: anyway:

i.operands - 1 - (t->opcode_space == SPACE_EVEXMAP4 && t->opcode_modifier.vexvvvv)

(suitably line wrapped of course).

> +	      switch (i.dir_encoding)
> +		{
> +		case dir_encoding_load:
> +		  if (operand_type_check (operand_types[isMemOperand], anymem)
> +		      || t->opcode_modifier.regmem)
> +		    goto check_reverse;
> +		  break;
>  
> -	      case dir_encoding_store:
> -		if (!operand_type_check (operand_types[i.operands - 1], anymem)
> -		    && !t->opcode_modifier.regmem)
> -		  goto check_reverse;
> -		break;
> +		case dir_encoding_store:
> +		  if (!operand_type_check (operand_types[isMemOperand], anymem)
> +		      && !t->opcode_modifier.regmem)
> +		    goto check_reverse;
> +		  break;
>  
> -	      case dir_encoding_swap:
> -		goto check_reverse;
> +		case dir_encoding_swap:
> +		  goto check_reverse;
>  
> -	      case dir_encoding_default:
> -		break;
> -	      }
> +		case dir_encoding_default:
> +		  break;
> +		}
> +	    }
>  	  /* If we want store form, we skip the current load.  */
>  	  if ((i.dir_encoding == dir_encoding_store
>  	       || i.dir_encoding == dir_encoding_swap)
> @@ -7432,11 +7443,13 @@ match_template (char mnem_suffix)
>  		continue;
>  	      /* Try reversing direction of operands.  */
>  	      j = is_cpu (t, CpuFMA4)
> -		  || is_cpu (t, CpuXOP) ? 1 : i.operands - 1;
> +		  || is_cpu (t, CpuXOP)
> +		  || is_cpu (t, CpuAPX_F) ? 1 : i.operands - 1;
>  	      overlap0 = operand_type_and (i.types[0], operand_types[j]);
>  	      overlap1 = operand_type_and (i.types[j], operand_types[0]);
>  	      overlap2 = operand_type_and (i.types[1], operand_types[1]);
> -	      gas_assert (t->operands != 3 || !check_register);
> +	      gas_assert (t->operands != 3 || !check_register
> +			  || is_cpu (t,CpuAPX_F));

Nit: Missing blank again.

> @@ -7471,6 +7484,12 @@ match_template (char mnem_suffix)
>  		  found_reverse_match = Opcode_VexW;
>  		  goto check_operands_345;
>  		}
> +	      else if (is_cpu (t,CpuAPX_F)
> +		       && i.operands == 3)
> +		{
> +		  found_reverse_match = Opcode_APX_NDDD;
> +		  goto check_operands_345;
> +		}
>  	      else if (t->opcode_space != SPACE_BASE
>  		       && (t->opcode_space != SPACE_0F
>  			   /* MOV to/from CR/DR/TR, as an exception, follow
> @@ -7636,6 +7655,15 @@ match_template (char mnem_suffix)
>  	 flipping VEX.W.  */
>        i.tm.opcode_modifier.vexw ^= VEXW0 ^ VEXW1;
>  
> +      j = i.tm.operand_types[0].bitfield.imm8;
> +      i.tm.operand_types[j] = operand_types[j + 1];
> +      i.tm.operand_types[j + 1] = operand_types[j];
> +      break;

I'm not overly happy to see this code getting duplicated. Are there any
encodings at all which have D and and immediate operand? I don't think
so, in which case this at least wants simplifying. But read on.

> +    case Opcode_APX_NDDD:
> +      /* Only the first two register operands need reversing.  */
> +      i.tm.base_opcode ^= 0x2;

I think you mean Opcode_D here?

>        j = i.tm.operand_types[0].bitfield.imm8;
>        i.tm.operand_types[j] = operand_types[j + 1];
>        i.tm.operand_types[j + 1] = operand_types[j];

Taking both remarks together, do we need Opcode_APX_NDDD at all? Can't
you use the ordinary Opcode_D, with

    default:
      /* If we found a reverse match we must alter the opcode direction
	 bit and clear/flip the regmem modifier one.  found_reverse_match
	 holds bits to change (different for int & float insns).  */

      i.tm.base_opcode ^= found_reverse_match;

      if (i.tm.opcode_space == SPACE_EVEXMAP4 && i.operands == 3)
        goto swap_first_2;
    ...
    swap_first_2:
      j = i.tm.operand_types[0].bitfield.imm8;
      i.tm.operand_types[j] = operand_types[j + 1];
      i.tm.operand_types[j + 1] = operand_types[j];
      break;

? (I'm not convinced the i.operands == 3 part of the condition is needed;
if at all possible it wants omitting.)

> @@ -8462,8 +8490,8 @@ process_operands (void)
>    const reg_entry *default_seg = NULL;
>  
>    /* We only need to check those implicit registers for instructions
> -     with 3 operands or less.  */
> -  if (i.operands <= 3)
> +     with 4 operands or less.  */
> +  if (i.operands <= 4)
>      for (unsigned int j = 0; j < i.operands; j++)
>        if (i.types[j].bitfield.instance != InstanceNone)
>  	i.reg_operands--;

How useful is it to keep the outer if() when 4-operand insns now also
need checking? There are extremely few 5-operand ones ...

> @@ -8825,6 +8853,9 @@ build_modrm_byte (void)
>        break;
>    if (v >= dest)
>      v = ~0;
> +  if (i.tm.opcode_space == SPACE_EVEXMAP4
> +      && i.tm.opcode_modifier.vexvvvv)
> +    v = dest;
>    if (i.tm.extension_opcode != None)
>      {
>        if (dest != source)
> @@ -9088,6 +9119,9 @@ build_modrm_byte (void)
>        set_rex_vrex (i.op[op].regs, REX_B, false);
>  	}
>  
> +      if (i.tm.opcode_space == SPACE_EVEXMAP4
> +	  && i.tm.opcode_modifier.vexvvvv)
> +	dest--;
>        if (op == dest)
>  	dest = ~0;
>        if (op == source)

These two changes are at the very least problematic with .insn, whose
behavior may not change. I'd also prefer if we could get away with just
one change to the function. Did you consider alternatives? We could re-
widen VexVVVV, such that the value 2 indicates that the destination is
encoded there. That then also has no chance of conflicting with .insn.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd.s
> @@ -0,0 +1,156 @@
> +# Check 64bit APX NDD instructions with evex prefix encoding
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +inc    %rax,%rbx
> +inc    %r31,%r8
> +inc    %r31,%r16
> +add    %r31b,%r8b,%r16b
> +addb    %r31b,%r8b,%r16b
> +add    %r31,%r8,%r16
> +addq    %r31,%r8,%r16
> +add    %r31d,%r8d,%r16d
> +addl    %r31d,%r8d,%r16d
> +add    %r31w,%r8w,%r16w
> +addw    %r31w,%r8w,%r16w
> +{store} add    %r31,%r8,%r16
> +{load}  add    %r31,%r8,%r16
> +add    %r31,(%r8),%r16
> +add    (%r31),%r8,%r16
> +add    0x9090(%r31,%r16,1),%r8,%r16
> +add    %r31,(%r8,%r16,8),%r16
> +add    $0x34,%r13b,%r17b
> +addl   $0x11,(%r19,%rax,4),%r20d
> +add    $0x1234,%ax,%r30w
> +add    $0x12344433,%r15,%r16
> +addq   $0x12344433,(%r15,%rcx,4),%r16
> +add    $0xfffffffff4332211,%rax,%r8
> +dec    %rax,%r17
> +decb   (%r31,%r12,1),%r8b
> +not    %rax,%r17
> +notb   (%r31,%r12,1),%r8b
> +neg    %rax,%r17
> +negb   (%r31,%r12,1),%r8b
> +sub    %r15b,%r17b,%r18b
> +sub    %r15d,(%r8),%r18d
> +sub    (%r15,%rax,1),%r16b,%r8b
> +sub    (%r15,%rax,1),%r16w,%r8w
> +subl   $0x11,(%r19,%rax,4),%r20d
> +sub    $0x1234,%ax,%r30w
> +sbb    %r15b,%r17b,%r18b
> +sbb    %r15d,(%r8),%r18d
> +sbb    (%r15,%rax,1),%r16b,%r8b
> +sbb    (%r15,%rax,1),%r16w,%r8w
> +sbbl   $0x11,(%r19,%rax,4),%r20d
> +sbb    $0x1234,%ax,%r30w
> +adc    %r15b,%r17b,%r18b
> +adc    %r15d,(%r8),%r18d
> +adc    (%r15,%rax,1),%r16b,%r8b
> +adc    (%r15,%rax,1),%r16w,%r8w
> +adcl   $0x11,(%r19,%rax,4),%r20d
> +adc    $0x1234,%ax,%r30w
> +or     %r15b,%r17b,%r18b
> +or     %r15d,(%r8),%r18d
> +or     (%r15,%rax,1),%r16b,%r8b
> +or     (%r15,%rax,1),%r16w,%r8w
> +orl    $0x11,(%r19,%rax,4),%r20d
> +or     $0x1234,%ax,%r30w
> +xor    %r15b,%r17b,%r18b
> +xor    %r15d,(%r8),%r18d
> +xor    (%r15,%rax,1),%r16b,%r8b
> +xor    (%r15,%rax,1),%r16w,%r8w
> +xorl   $0x11,(%r19,%rax,4),%r20d
> +xor    $0x1234,%ax,%r30w
> +and    %r15b,%r17b,%r18b
> +and    %r15d,(%r8),%r18d
> +and    (%r15,%rax,1),%r16b,%r8b
> +and    (%r15,%rax,1),%r16w,%r8w
> +andl   $0x11,(%r19,%rax,4),%r20d
> +and    $0x1234,%ax,%r30w
> +rorb   (%rax),%r31b
> +ror    $0x2,%r12b,%r31b
> +rorl   $0x2,(%rax),%r31d
> +rorw   (%rax),%r31w
> +ror    %cl,%r16b,%r8b
> +rorw   %cl,(%r19,%rax,4),%r31w
> +rolb   (%rax),%r31b
> +rol    $0x2,%r12b,%r31b
> +roll   $0x2,(%rax),%r31d
> +rolw   (%rax),%r31w
> +rol    %cl,%r16b,%r8b
> +rolw   %cl,(%r19,%rax,4),%r31w
> +rcrb   (%rax),%r31b
> +rcr    $0x2,%r12b,%r31b
> +rcrl   $0x2,(%rax),%r31d
> +rcrw   (%rax),%r31w
> +rcr    %cl,%r16b,%r8b
> +rcrw   %cl,(%r19,%rax,4),%r31w
> +rclb   (%rax),%r31b
> +rcl    $0x2,%r12b,%r31b
> +rcll   $0x2,(%rax),%r31d
> +rclw   (%rax),%r31w
> +rcl    %cl,%r16b,%r8b
> +rclw   %cl,(%r19,%rax,4),%r31w
> +shlb   (%rax),%r31b
> +shl    $0x2,%r12b,%r31b
> +shll   $0x2,(%rax),%r31d
> +shlw   (%rax),%r31w
> +shl    %cl,%r16b,%r8b
> +shlw   %cl,(%r19,%rax,4),%r31w
> +sarb   (%rax),%r31b
> +sar    $0x2,%r12b,%r31b
> +sarl   $0x2,(%rax),%r31d
> +sarw   (%rax),%r31w
> +sar    %cl,%r16b,%r8b
> +sarw   %cl,(%r19,%rax,4),%r31w
> +shlb   (%rax),%r31b
> +shl    $0x2,%r12b,%r31b
> +shll   $0x2,(%rax),%r31d
> +shlw   (%rax),%r31w
> +shl    %cl,%r16b,%r8b
> +shlw   %cl,(%r19,%rax,4),%r31w
> +shrb   (%rax),%r31b
> +shr    $0x2,%r12b,%r31b
> +shrl   $0x2,(%rax),%r31d
> +shrw   (%rax),%r31w
> +shr    %cl,%r16b,%r8b
> +shrw   %cl,(%r19,%rax,4),%r31w
> +shld   $0x1,%r12,(%rax),%r31
> +shld   $0x2,%r8w,%r12w,%r31w
> +shld   $0x2,%r15d,(%rax),%r31d
> +shld   %cl,%r9w,(%rax),%r31w
> +shld   %cl,%r12,%r16,%r8
> +shld   %cl,%r13w,(%r19,%rax,4),%r31w
> +shrd   $0x1,%r12,(%rax),%r31
> +shrd   $0x2,%r8w,%r12w,%r31w
> +shrd   $0x2,%r15d,(%rax),%r31d
> +shrd   %cl,%r9w,(%rax),%r31w
> +shrd   %cl,%r12,%r16,%r8
> +shrd   %cl,%r13w,(%r19,%rax,4),%r31w
> +adcx   %r15d,%r8d,%r18d
> +adcx   (%r15,%r31,1),%r8d,%r18d
> +adcx   (%r15,%r31,1),%r8
> +adox   %r15d,%r8d,%r18d
> +adox   (%r15,%r31,1),%r8d,%r18d
> +adox   (%r15,%r31,1),%r8
> +cmovo  0x90909090(%eax),%edx,%r8d
> +cmovno 0x90909090(%eax),%edx,%r8d
> +cmovb  0x90909090(%eax),%edx,%r8d
> +cmovae 0x90909090(%eax),%edx,%r8d
> +cmove  0x90909090(%eax),%edx,%r8d
> +cmovne 0x90909090(%eax),%edx,%r8d
> +cmovbe 0x90909090(%eax),%edx,%r8d
> +cmova  0x90909090(%eax),%edx,%r8d
> +cmovs  0x90909090(%eax),%edx,%r8d
> +cmovns 0x90909090(%eax),%edx,%r8d
> +cmovp  0x90909090(%eax),%edx,%r8d
> +cmovnp 0x90909090(%eax),%edx,%r8d
> +cmovl  0x90909090(%eax),%edx,%r8d
> +cmovge 0x90909090(%eax),%edx,%r8d
> +cmovle 0x90909090(%eax),%edx,%r8d
> +cmovg  0x90909090(%eax),%edx,%r8d
> +imul   0x90909(%eax),%edx,%r8d
> +imul   0x909(%rax,%r31,8),%rdx,%r25

What about imul by immediate? The present spec is quite unclear there:
The insn page says {ND=ZU} and the table says 0/1 in the ND column.

> +.byte 0x62,0xf4,0xfc,0x08,0xff,0xc0  #inc %rax
> +.byte 0x62,0xf4,0xec,0x08,0xff,0xc0  #bad

As before, please avoid .byte whenever possible. And please have a more
detailed comment as to what is being encoded, when .byte cannot be
avoided. Plus, if at all possible, have "bad" tests live in separate
testcases from "good" ones.

> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h

Once again I'll reply to disassembler changes separately.

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -960,6 +960,7 @@ typedef struct insn_template
>  /* The next value is arbitrary, as long as it's non-zero and distinct
>     from all other values above.  */
>  #define Opcode_VexW	0xf /* Operand order controlled by VEX.W. */
> +#define Opcode_APX_NDDD	0x11 /* Direction bit for APX NDD insns. */

The comment talks of a single bit, but the value has two bits set.
Plus in the code you also don't use this constant as described by the
comment. Aiui like for Opcode_VexW the value is really arbitrary, just
as long as it's different from others. In which case I'd rather
suggest using e.g. 0xe (if, unlike suggested above, Opcode_D cannot be
re-used).

Also I don't think there's a need for three D-s in the name.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl

Comments given on the earlier patch apply here (and elsewhere) as well.

Jan