[4/8] Support APX NDD

  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
  

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
  

On 19.09.2023 17:25, Cui, Lili wrote:
> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h
> @@ -353,8 +353,8 @@
>    /* PREFIX_EVEX_MAP4_66 */
>    {
>      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> -    { "adoxS",	{ Gdq, Edq }, 0 },
> -    { "adcxS",	{ Gdq, Edq }, 0 },
> +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },

With the OP_VEX() change moved to the earlier patch, you wouldn't need to
alter again right away what was just added there.

> --- a/opcodes/i386-dis-evex-reg.h
> +++ b/opcodes/i386-dis-evex-reg.h
> @@ -56,6 +56,105 @@
>      { "blsmskS",	{ VexGdq, Edq }, 0 },
>      { "blsiS",		{ VexGdq, Edq }, 0 },
>    },
> +  /* REG_EVEX_MAP4_80 */
> +  {
> +    { "addA",	{ VexGb, Eb, Ib }, 0 },
> +    { "orA",	{ VexGb, Eb, Ib }, 0 },
> +    { "adcA",	{ VexGb, Eb, Ib }, 0 },
> +    { "sbbA",	{ VexGb, Eb, Ib }, 0 },

Aren't these two and other adc/sbb entries required already in the earlier
patch, for consistency with what you add there on the assembler side?

> +    { "andA",	{ VexGb, Eb, Ib }, 0 },
> +    { "subA",	{ VexGb, Eb, Ib }, 0 },
> +    { "xorA",	{ VexGb, Eb, Ib }, 0 },
> +    { Bad_Opcode },
> +  },
> +  /* REG_EVEX_MAP4_81 */
> +  {
> +    { "addQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "orQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "adcQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "sbbQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "andQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "subQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "xorQ",	{ VexGv, Ev, Iv }, 0 },
> +    { Bad_Opcode },
> +  },
> +  /* REG_EVEX_MAP4_83 */
> +  {
> +    { "addQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "orQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "adcQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "sbbQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "andQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "subQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "xorQ",	{ VexGv, Ev, sIb }, 0 },
> +    { Bad_Opcode },
> +  },

While these are needed because of the differences (from existing table
entries we have) in the last entry (albeit I don't think those last
entries need to actually be spelled out; you don't spell out trailing
invalid entries further down, and we don't do so elsewhere either), ...

> +  /* REG_EVEX_MAP4_C0 */
> +  {
> +    { "rolA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rorA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rclA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rcrA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shrA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> +    { "sarA",	{ VexGb, Eb, Ib }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_C1 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rorQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rclQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shrQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "sarQ",	{ VexGv, Ev, Ib }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D0 */
> +  {
> +    { "rolA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rorA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rclA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rcrA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shrA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> +    { "sarA",	{ VexGb, Eb, I1 }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D1 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rorQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rclQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shrQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "sarQ",	{ VexGv, Ev, I1 }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D2 */
> +  {
> +    { "rolA",	{ VexGb, Eb, CL }, 0 },
> +    { "rorA",	{ VexGb, Eb, CL }, 0 },
> +    { "rclA",	{ VexGb, Eb, CL }, 0 },
> +    { "rcrA",	{ VexGb, Eb, CL }, 0 },
> +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> +    { "shrA",	{ VexGb, Eb, CL }, 0 },
> +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> +    { "sarA",	{ VexGb, Eb, CL }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D3 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rorQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rclQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shrQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> +    { "sarQ",	{ VexGv, Ev, CL }, 0 },
> +  },

... do we really need all these new entries? OP_VEX() checks need_vex first
thing, so simply adjusting and then re-using the existing entries would seem
possible.

> @@ -9070,6 +9085,14 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	  ins->rex &= ~REX_B;
>  	  ins->rex2 &= ~REX_R;
>  	}
> +      if (ins->evex_type == evex_from_legacy)
> +	{
> +	  if (ins->vex.ll || ins->vex.zeroing
> +	      || (!ins->vex.b && (ins->vex.register_specifier
> +				  || !ins->vex.v)))
> +	    return &bad_opcode;

Don't these checks also apply to evex_from_vex?

> +	  ins->rex |= REX_OPCODE;

This, otoh, may truly be evex_from_legacy-only (but I'm not entirely
certain).

> @@ -9080,7 +9103,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	return &err_opcode;
>  
>        /* Set vector length.  */
> -      if (ins->modrm.mod == 3 && ins->vex.b)
> +      if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type == evex_default)
>  	ins->vex.length = 512;

Down from here, still in the same function, there's another vex.b
check which I think also wants qualifying.

> @@ -10994,7 +11017,7 @@ print_displacement (instr_info *ins, bfd_signed_vma val)
>  static void
>  intel_operand_size (instr_info *ins, int bytemode, int sizeflag)
>  {
> -  if (ins->vex.b)
> +  if (ins->vex.b && ins->evex_type != evex_from_legacy)

Wouldn't this better be ins->evex_type == evex_default, ...

> @@ -11928,7 +11951,8 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
>  	ins->vex.no_broadcast = true;
>  
>        if (!ins->vex.no_broadcast
> -	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used)))
> +	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used))
> +	  && ins->evex_type == evex_default)

... just like you have it here?

However, for this change, doesn't this need moving to the enclosing if()?
You should accidentally set EVEX_b_used here for APX insns.

> @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
>    if (!ins->need_vex)
>      return true;
>  
> +  if (ins->evex_type == evex_from_legacy)
> +    {
> +      if (ins->vex.b)
> +	ins->evex_used |= EVEX_b_used;
> +      else
> +	 return true;
> +    }

When you reuse fields or definitions in places where their names don't
match their purpose (the field dealt with here is "nd" after all, not
"b"), a comment wants adding. There's also something odd with
indentation here, but I suppose an if/else construct isn't needed in
the first place.

Jan
  

> >  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"?
>
 
Done.

> 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.
> 
You're right, deleted "i.operands == 3".

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

Done.

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

Changed to MemOperand.
 
> > +				
> 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).
> 

Done.

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

Done.

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

Your suggestion is indeed better than before. It worked without "i.operands == 3".

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

Deleted it.

> > @@ -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.
> 
I added value 2 for NDD, if it's ok, I will create another patch to move  (i.tm.extension_opcode != None) to VexVVVVDEST branch, and use value 3 instead of SWAP_SOURCES, maybe name it VexVVVVSRC1, or just VexVVVVOP1, VexVVVVOP2 and VexVVVVOP3?

  /* How to encode VEX.vvvv:
     0: VEX.vvvv must be 1111b.
     1: VEX.vvvv encodes one of the register operands.
     2: VEX.vvvv encodes as the dest register operands.
   */
#define VexVVVVSRC   1
#define VexVVVVDEST  2
  VexVVVV,


  if (i.tm.opcode_modifier.vexvvvv == VexVVVVDEST)
    {
      v = dest;
      dest-- ;
    }
  else if (i.tm.opcode_modifier.vexvvvv == VexVVVVSRC)
    {
      v = source + 1;
      for (v = source + 1; v < dest; ++v)
        if (v != reg_slot)
          break;
      if (i.tm.extension_opcode != None)
        {
          if (dest != source)
            v = dest;
          dest = ~0;
        }
      gas_assert (source < dest);
      if (i.tm.opcode_modifier.operandconstraint == SWAP_SOURCES
          && source != op)
        {
          unsigned int tmp = source;

          source = v;
          v = tmp;
        }
    }
  else
    v = ~0; 

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

We don't support it yet, I put it in RFC.
...
2. Support APX ZU   -- In progress
3. Support APX CCMP and CTEST -- In progress
...

About 0/1 in the ND column, it means ZU can be 0/1.

IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
Although these instructions do not support NDD, the EVEX.ND bit is used to control whether its
destination register has its upper bits (namely, bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise, they keep the old values
when OSIZE is 8b or 16b. For these instructions, EVEX.[V4,V3,V2,V1,V0] must be all zero.

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

This case wants to test that inc supports evex format without GPR32,  patch part II 1/6 will cover it. The first case has been removed and the second case has been added to x86-64-apx-evex-promoted-bad.s.
 
> > --- a/opcodes/i386-dis-evex-prefix.h
> > +++ b/opcodes/i386-dis-evex-prefix.h
> 
> Once again I'll reply to disassembler changes separately.
> 

Ok.

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

Deleted Opcode_APX_NDDD.

> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> 
> Comments given on the earlier patch apply here (and elsewhere) as well.
> 
> Jan
  

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 19.09.2023 17:25, Cui, Lili wrote:
> > --- a/opcodes/i386-dis-evex-prefix.h
> > +++ b/opcodes/i386-dis-evex-prefix.h
> > @@ -353,8 +353,8 @@
> >    /* PREFIX_EVEX_MAP4_66 */
> >    {
> >      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> > -    { "adoxS",	{ Gdq, Edq }, 0 },
> > -    { "adcxS",	{ Gdq, Edq }, 0 },
> > +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> > +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },
> 
> With the OP_VEX() change moved to the earlier patch, you wouldn't need to
> alter again right away what was just added there.
>
Hi Jan,

I just wanted to move the instructions in EVEX involving NDD into this patch. I think they should belong to this NDD patch.  As in your second comment, I also found some instructions for the disassembler missing in the APX evex patch. 

Lili.

> > --- a/opcodes/i386-dis-evex-reg.h
> > +++ b/opcodes/i386-dis-evex-reg.h
> > @@ -56,6 +56,105 @@
> >      { "blsmskS",	{ VexGdq, Edq }, 0 },
> >      { "blsiS",		{ VexGdq, Edq }, 0 },
> >    },
> > +  /* REG_EVEX_MAP4_80 */
> > +  {
> > +    { "addA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "orA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "adcA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "sbbA",	{ VexGb, Eb, Ib }, 0 },
> 
> Aren't these two and other adc/sbb entries required already in the earlier
> patch, for consistency with what you add there on the assembler side?
>
  

On 22.10.2023 16:05, Cui, Lili wrote:
>>> @@ -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.
>>
> I added value 2 for NDD, if it's ok, I will create another patch to move  (i.tm.extension_opcode != None) to VexVVVVDEST branch, and use value 3 instead of SWAP_SOURCES, maybe name it VexVVVVSRC1, or just VexVVVVOP1, VexVVVVOP2 and VexVVVVOP3?

Hard to tell without actually seeing the code. What you did quote below
used just SRC/DEST.

>   /* How to encode VEX.vvvv:
>      0: VEX.vvvv must be 1111b.
>      1: VEX.vvvv encodes one of the register operands.
>      2: VEX.vvvv encodes as the dest register operands.
>    */
> #define VexVVVVSRC   1
> #define VexVVVVDEST  2
>   VexVVVV,

For readability maybe insert underscores (and if SRC remains un-numbered,
maybe also DST in place of DEST)?

>>> --- /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:
>>> +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.
>>
> 
> We don't support it yet, I put it in RFC.
> ...
> 2. Support APX ZU   -- In progress
> 3. Support APX CCMP and CTEST -- In progress
> ...
> 
> About 0/1 in the ND column, it means ZU can be 0/1.
> 
> IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
> Although these instructions do not support NDD, the EVEX.ND bit is used to control whether its
> destination register has its upper bits (namely, bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
> That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise, they keep the old values
> when OSIZE is 8b or 16b. For these instructions, EVEX.[V4,V3,V2,V1,V0] must be all zero.

So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is EVEX.ND
then simply being ignored? The ZU really is meaningful only for 16-bit forms,
aiui ...

>>> +.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.
>>
> 
> This case wants to test that inc supports evex format without GPR32,  patch part II 1/6 will cover it. The first case has been removed and the second case has been added to x86-64-apx-evex-promoted-bad.s.

That's all fine, but still wants expressing with .insn rather than .byte,
if at all possible.

Jan
  

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 19.09.2023 17:25, Cui, Lili wrote:
> > --- a/opcodes/i386-dis-evex-prefix.h
> > +++ b/opcodes/i386-dis-evex-prefix.h
> > @@ -353,8 +353,8 @@
> >    /* PREFIX_EVEX_MAP4_66 */
> >    {
> >      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> > -    { "adoxS",	{ Gdq, Edq }, 0 },
> > -    { "adcxS",	{ Gdq, Edq }, 0 },
> > +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> > +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },
> 
> With the OP_VEX() change moved to the earlier patch, you wouldn't need to
> alter again right away what was just added there.
> 

As I commented in another email, I want to move instructions like adcx into this NDD patch. Do you think it is ok?

> > --- a/opcodes/i386-dis-evex-reg.h
> > +++ b/opcodes/i386-dis-evex-reg.h
> > @@ -56,6 +56,105 @@
> >      { "blsmskS",	{ VexGdq, Edq }, 0 },
> >      { "blsiS",		{ VexGdq, Edq }, 0 },
> >    },
> > +  /* REG_EVEX_MAP4_80 */
> > +  {
> > +    { "addA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "orA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "adcA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "sbbA",	{ VexGb, Eb, Ib }, 0 },
> 
> Aren't these two and other adc/sbb entries required already in the earlier
> patch, for consistency with what you add there on the assembler side?
> 
> > +    { "andA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "subA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "xorA",	{ VexGb, Eb, Ib }, 0 },
> > +    { Bad_Opcode },
> > +  },
> > +  /* REG_EVEX_MAP4_81 */
> > +  {
> > +    { "addQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "orQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "adcQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "sbbQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "andQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "subQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { "xorQ",	{ VexGv, Ev, Iv }, 0 },
> > +    { Bad_Opcode },
> > +  },
> > +  /* REG_EVEX_MAP4_83 */
> > +  {
> > +    { "addQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "orQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "adcQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "sbbQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "andQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "subQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { "xorQ",	{ VexGv, Ev, sIb }, 0 },
> > +    { Bad_Opcode },
> > +  },
> 
> While these are needed because of the differences (from existing table entries
> we have) in the last entry (albeit I don't think those last entries need to
> actually be spelled out; you don't spell out trailing invalid entries further
> down, and we don't do so elsewhere either), ...

Done.

> 
> > +  /* REG_EVEX_MAP4_C0 */
> > +  {
> > +    { "rolA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "rorA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "rclA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "rcrA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "shrA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> > +    { "sarA",	{ VexGb, Eb, Ib }, 0 },
> > +  },
> > +  /* REG_EVEX_MAP4_C1 */
> > +  {
> > +    { "rolQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "rorQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "rclQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "rcrQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "shrQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> > +    { "sarQ",	{ VexGv, Ev, Ib }, 0 },
> > +  },
> > +  /* REG_EVEX_MAP4_D0 */
> > +  {
> > +    { "rolA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "rorA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "rclA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "rcrA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "shrA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> > +    { "sarA",	{ VexGb, Eb, I1 }, 0 },
> > +  },
> > +  /* REG_EVEX_MAP4_D1 */
> > +  {
> > +    { "rolQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "rorQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "rclQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "rcrQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "shrQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> > +    { "sarQ",	{ VexGv, Ev, I1 }, 0 },
> > +  },
> > +  /* REG_EVEX_MAP4_D2 */
> > +  {
> > +    { "rolA",	{ VexGb, Eb, CL }, 0 },
> > +    { "rorA",	{ VexGb, Eb, CL }, 0 },
> > +    { "rclA",	{ VexGb, Eb, CL }, 0 },
> > +    { "rcrA",	{ VexGb, Eb, CL }, 0 },
> > +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> > +    { "shrA",	{ VexGb, Eb, CL }, 0 },
> > +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> > +    { "sarA",	{ VexGb, Eb, CL }, 0 },
> > +  },
> > +  /* REG_EVEX_MAP4_D3 */
> > +  {
> > +    { "rolQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "rorQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "rclQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "rcrQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "shrQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> > +    { "sarQ",	{ VexGv, Ev, CL }, 0 },
> > +  },
> 
> ... do we really need all these new entries? OP_VEX() checks need_vex first
> thing, so simply adjusting and then re-using the existing entries would seem
> possible.
> 

Done.

> > @@ -9070,6 +9085,14 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >  	  ins->rex &= ~REX_B;
> >  	  ins->rex2 &= ~REX_R;
> >  	}
> > +      if (ins->evex_type == evex_from_legacy)
> > +	{
> > +	  if (ins->vex.ll || ins->vex.zeroing
> > +	      || (!ins->vex.b && (ins->vex.register_specifier
> > +				  || !ins->vex.v)))
> > +	    return &bad_opcode;
> 
> Don't these checks also apply to evex_from_vex?

Oh, I've duplicated some of the checks in the above code and removed the duplicated parts.

     if (ins->evex_type == evex_from_legacy)
        {
          /* 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))
            return &bad_opcode;
          ins->rex |= REX_OPCODE;
        }

      /* EVEX from legacy instructions require that EVEX.L’L, EVEX.z and the
         lower 2 bits of EVEX.aaa must be 0.
         EVEX from evex instrucions require that EVEX.L’L and the lower 2 bits of
         EVEX.aaa must be 0.  */
      if (ins->evex_type == evex_from_legacy || ins->evex_type == evex_from_vex)
        {
          if ((*ins->codep & 0x3) != 0
              || (*ins->codep >> 6 & 0x3) != 0
              || (ins->evex_type == evex_from_legacy
                  && (*ins->codep >> 5 & 0x1) != 0)
              || (ins->evex_type == evex_from_vex
                  && !ins->vex.b))
            return &bad_opcode;
        }
> 
> > +	  ins->rex |= REX_OPCODE;
> 
> This, otoh, may truly be evex_from_legacy-only (but I'm not entirely certain).
>

Added after separation.
 
> > @@ -9080,7 +9103,7 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >  	return &err_opcode;
> >
> >        /* Set vector length.  */
> > -      if (ins->modrm.mod == 3 && ins->vex.b)
> > +      if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type ==
> > + evex_default)
> >  	ins->vex.length = 512;
> 
> Down from here, still in the same function, there's another vex.b check which I
> think also wants qualifying.
> 

Not found in this function, but found a place that needs to be qualified in print_insn.

          /* Check whether rounding control was enabled for an insn not
             supporting it.  */
          if (ins.modrm.mod == 3 && ins.vex.b
              && !(ins.evex_used & EVEX_b_used))
            {
              for (i = 0; i < MAX_OPERANDS; ++i)
                {
                  ins.obufp = ins.op_out[i];
                  if (*ins.obufp)
                    continue;
                  oappend (&ins, names_rounding[ins.vex.ll]);
                  oappend (&ins, "bad}");
                  break;
                }
            }

> > @@ -10994,7 +11017,7 @@ print_displacement (instr_info *ins,
> > bfd_signed_vma val)  static void  intel_operand_size (instr_info *ins,
> > int bytemode, int sizeflag)  {
> > -  if (ins->vex.b)
> > +  if (ins->vex.b && ins->evex_type != evex_from_legacy)
> 
> Wouldn't this better be ins->evex_type == evex_default, ...
> 

Done.

> > @@ -11928,7 +11951,8 @@ OP_E_memory (instr_info *ins, int bytemode,
> int sizeflag)
> >  	ins->vex.no_broadcast = true;
> >
> >        if (!ins->vex.no_broadcast
> > -	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used)))
> > +	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used))
> > +	  && ins->evex_type == evex_default)
> 
> ... just like you have it here?
> 
> However, for this change, doesn't this need moving to the enclosing if()?
> You should accidentally set EVEX_b_used here for APX insns.
> 

Sure, moved it.

> > @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int
> sizeflag ATTRIBUTE_UNUSED)
> >    if (!ins->need_vex)
> >      return true;
> >
> > +  if (ins->evex_type == evex_from_legacy)
> > +    {
> > +      if (ins->vex.b)
> > +	ins->evex_used |= EVEX_b_used;
> > +      else
> > +	 return true;
> > +    }
> 
> When you reuse fields or definitions in places where their names don't match
> their purpose (the field dealt with here is "nd" after all, not "b"), a comment
> wants adding. There's also something odd with indentation here, but I
> suppose an if/else construct isn't needed in the first place.
> 
Added comment and adjusted the formatting, I think we need if/else like "adox " which supports EVEX.ND=0/1.

/* PREFIX_0F38F6 */
  {
    { "wrssK",  { M, Gdq }, 0 },
    { "adoxS",  { VexGdq, Gdq, Edq}, 0 },

adox, 0xf366, ADX|APX_F, Modrm|CheckOperandSize|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|EVex128|EVexMap4, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
adox, 0xf366, ADX|APX_F, Modrm|CheckOperandSize|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|VexVVVVDest|EVex128|EVexMap4, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }


Thanks,
Lili.
  

On 24.10.2023 13:39, Cui, Lili wrote:
>> Subject: Re: [PATCH 4/8] Support APX NDD
>>
>> On 19.09.2023 17:25, Cui, Lili wrote:
>>> --- a/opcodes/i386-dis-evex-prefix.h
>>> +++ b/opcodes/i386-dis-evex-prefix.h
>>> @@ -353,8 +353,8 @@
>>>    /* PREFIX_EVEX_MAP4_66 */
>>>    {
>>>      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
>>> -    { "adoxS",	{ Gdq, Edq }, 0 },
>>> -    { "adcxS",	{ Gdq, Edq }, 0 },
>>> +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
>>> +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },
>>
>> With the OP_VEX() change moved to the earlier patch, you wouldn't need to
>> alter again right away what was just added there.
>>
> 
> As I commented in another email, I want to move instructions like adcx into this NDD patch. Do you think it is ok?

Yeah, having ND-capable insns in the ND patch makes sense. I don't care
overly much about the actual split. It wants to be half-way consistent,
and beyond that - as said - it helps if code/data added by earlier
patches doesn't need re-writing altogether in later ones.

>>> @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int
>> sizeflag ATTRIBUTE_UNUSED)
>>>    if (!ins->need_vex)
>>>      return true;
>>>
>>> +  if (ins->evex_type == evex_from_legacy)
>>> +    {
>>> +      if (ins->vex.b)
>>> +	ins->evex_used |= EVEX_b_used;
>>> +      else
>>> +	 return true;
>>> +    }
>>
>> When you reuse fields or definitions in places where their names don't match
>> their purpose (the field dealt with here is "nd" after all, not "b"), a comment
>> wants adding. There's also something odd with indentation here, but I
>> suppose an if/else construct isn't needed in the first place.
>>
> Added comment and adjusted the formatting, I think we need if/else like "adox " which supports EVEX.ND=0/1.

You will need if(), sure, but I'm not convinced of "else". Constructs
like

    if (cond)
      return;
    else
      whatever;

bother me in general, as adding (even if just slightly) more mental load
for the reader. Just

    if (cond)
      return;
    whatever;

is all you need: Less text, less indentation. In the specific case
above yet another possibility may exist (without having actually tried
it):

  if (ins->evex_type == evex_from_legacy)
    {
      ins->evex_used |= EVEX_b_used;
      if (!ins->vex.b)
	 return true;
    }

I.e. this would be as long as setting EVEX_b_used without EVEX.b set
doesn't cause any issues.

Jan
  

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 22.10.2023 16:05, Cui, Lili wrote:
> >>> @@ -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.
> >>
> > I added value 2 for NDD, if it's ok, I will create another patch to move
> (i.tm.extension_opcode != None) to VexVVVVDEST branch, and use value 3
> instead of SWAP_SOURCES, maybe name it VexVVVVSRC1, or just VexVVVVOP1,
> VexVVVVOP2 and VexVVVVOP3?
> 
> Hard to tell without actually seeing the code. What you did quote below used
> just SRC/DEST.
> 

Ok. Let review the current changes first.

> >   /* How to encode VEX.vvvv:
> >      0: VEX.vvvv must be 1111b.
> >      1: VEX.vvvv encodes one of the register operands.
> >      2: VEX.vvvv encodes as the dest register operands.
> >    */
> > #define VexVVVVSRC   1
> > #define VexVVVVDEST  2
> >   VexVVVV,
> 
> For readability maybe insert underscores (and if SRC remains un-numbered,
> maybe also DST in place of DEST)?

Done.

> 
> >>> --- /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:
> >>> +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.
> >>
> >
> > We don't support it yet, I put it in RFC.
> > ...
> > 2. Support APX ZU   -- In progress
> > 3. Support APX CCMP and CTEST -- In progress ...
> >
> > About 0/1 in the ND column, it means ZU can be 0/1.
> >
> > IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
> > Although these instructions do not support NDD, the EVEX.ND bit is
> > used to control whether its destination register has its upper bits (namely,
> bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
> > That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise,
> > they keep the old values when OSIZE is 8b or 16b. For these instructions,
> EVEX.[V4,V3,V2,V1,V0] must be all zero.
> 
> So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is EVEX.ND then
> simply being ignored? The ZU really is meaningful only for 16-bit forms, aiui ...
> 

EVEX.ZU should be ignored for 32-bit and 64-bit forms. For imul (in spec 6.30 IMUL), EVEX.ND stands for ND or ZU. I think ZU makes sense for both the 16-bit form (imul) and the 8-bit form (setcc, I'm not sure if imul supports it yet).

> >>> +.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.
> >>
> >
> > This case wants to test that inc supports evex format without GPR32,  patch
> part II 1/6 will cover it. The first case has been removed and the second case
> has been added to x86-64-apx-evex-promoted-bad.s.
> 
> That's all fine, but still wants expressing with .insn rather than .byte, if at all
> possible.
> 

Got your point, for the first case we can use .insn instead, for the second case need .byte. 

Thanks,
Lili.
  

On 25.10.2023 10:10, Cui, Lili wrote:
>> On 22.10.2023 16:05, Cui, Lili wrote:
>>>>> --- /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:
>>>>> +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.
>>>>
>>>
>>> We don't support it yet, I put it in RFC.
>>> ...
>>> 2. Support APX ZU   -- In progress
>>> 3. Support APX CCMP and CTEST -- In progress ...
>>>
>>> About 0/1 in the ND column, it means ZU can be 0/1.
>>>
>>> IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
>>> Although these instructions do not support NDD, the EVEX.ND bit is
>>> used to control whether its destination register has its upper bits (namely,
>> bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
>>> That is, if EVEX.ND = 1, the upper bits are always zeroed; otherwise,
>>> they keep the old values when OSIZE is 8b or 16b. For these instructions,
>> EVEX.[V4,V3,V2,V1,V0] must be all zero.
>>
>> So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is EVEX.ND then
>> simply being ignored? The ZU really is meaningful only for 16-bit forms, aiui ...
>>
> 
> EVEX.ZU should be ignored for 32-bit and 64-bit forms. For imul (in spec 6.30 IMUL), EVEX.ND stands for ND or ZU.

In cases like this, where ignoring bits is kind of unexpected, the spec
would better say explicitly (on the instruction page) when a meaningless
bit is indeed ignored, rather than being reserved and causing #UD. Note
how even the text in the APX-EVEX-INT section leaves open (or at least
ambiguous, by not mentioning the case) whether SETcc with a memory
operand ignores EVEX.ND or causes #UD when the bit is set.

> I think ZU makes sense for both the 16-bit form (imul) and the 8-bit form (setcc, I'm not sure if imul supports it yet).

No, IMUL by immediate (or actually any IMUL with multiple operands) doesn't
support byte register operands. For SETcc the ZU aspect is pretty clear
and doesn't even need expressing by new syntax in (dis)assembly - you can
simply distinguish the two forms by using either 8-bit registers (no ZU)
or 32-/64-bit ones (with ZU). In principle that's possible with IMUL as
well, of course, but it may be deemed a little odd:

	imul	$17, %dx, %cx
	imul	$17, %dx, %ecx

Yet personally I'd still prefer this over adding e.g. {zu} on either the
mnemonic or the destination operand. Question (as with the way to express
{nf}) is how other assemblers are going to handle it. (Would be quite nice
if the spec could at least give more clear hints towards suggested syntax,
but that hadn't been the case already with the syntax extensions needed
for AVX512.)

Jan
  

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 24.10.2023 13:39, Cui, Lili wrote:
> >> Subject: Re: [PATCH 4/8] Support APX NDD
> >>
> >> On 19.09.2023 17:25, Cui, Lili wrote:
> >>> --- a/opcodes/i386-dis-evex-prefix.h
> >>> +++ b/opcodes/i386-dis-evex-prefix.h
> >>> @@ -353,8 +353,8 @@
> >>>    /* PREFIX_EVEX_MAP4_66 */
> >>>    {
> >>>      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> >>> -    { "adoxS",	{ Gdq, Edq }, 0 },
> >>> -    { "adcxS",	{ Gdq, Edq }, 0 },
> >>> +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> >>> +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },
> >>
> >> With the OP_VEX() change moved to the earlier patch, you wouldn't
> >> need to alter again right away what was just added there.
> >>
> >
> > As I commented in another email, I want to move instructions like adcx into
> this NDD patch. Do you think it is ok?
> 
> Yeah, having ND-capable insns in the ND patch makes sense. I don't care overly
> much about the actual split. It wants to be half-way consistent, and beyond
> that - as said - it helps if code/data added by earlier patches doesn't need re-
> writing altogether in later ones.
> 

Ok.

> >>> @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int
> >> sizeflag ATTRIBUTE_UNUSED)
> >>>    if (!ins->need_vex)
> >>>      return true;
> >>>
> >>> +  if (ins->evex_type == evex_from_legacy)
> >>> +    {
> >>> +      if (ins->vex.b)
> >>> +	ins->evex_used |= EVEX_b_used;
> >>> +      else
> >>> +	 return true;
> >>> +    }
> >>
> >> When you reuse fields or definitions in places where their names
> >> don't match their purpose (the field dealt with here is "nd" after
> >> all, not "b"), a comment wants adding. There's also something odd
> >> with indentation here, but I suppose an if/else construct isn't needed in the
> first place.
> >>
> > Added comment and adjusted the formatting, I think we need if/else like
> "adox " which supports EVEX.ND=0/1.
> 
> You will need if(), sure, but I'm not convinced of "else". Constructs like
> 
>     if (cond)
>       return;
>     else
>       whatever;
> 
> bother me in general, as adding (even if just slightly) more mental load for the
> reader. Just
> 
>     if (cond)
>       return;
>     whatever;
> 
> is all you need: Less text, less indentation. In the specific case above yet
> another possibility may exist (without having actually tried
> it):
> 
>   if (ins->evex_type == evex_from_legacy)
>     {
>       ins->evex_used |= EVEX_b_used;
>       if (!ins->vex.b)
> 	 return true;
>     }
> 
> I.e. this would be as long as setting EVEX_b_used without EVEX.b set doesn't
> cause any issues.
> 

Done.

Thanks,
Lili.
  

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 25.10.2023 10:10, Cui, Lili wrote:
> >> On 22.10.2023 16:05, Cui, Lili wrote:
> >>>>> --- /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:
> >>>>> +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.
> >>>>
> >>>
> >>> We don't support it yet, I put it in RFC.
> >>> ...
> >>> 2. Support APX ZU   -- In progress
> >>> 3. Support APX CCMP and CTEST -- In progress ...
> >>>
> >>> About 0/1 in the ND column, it means ZU can be 0/1.
> >>>
> >>> IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
> >>> Although these instructions do not support NDD, the EVEX.ND bit is
> >>> used to control whether its destination register has its upper bits
> >>> (namely,
> >> bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
> >>> That is, if EVEX.ND = 1, the upper bits are always zeroed;
> >>> otherwise, they keep the old values when OSIZE is 8b or 16b. For
> >>> these instructions,
> >> EVEX.[V4,V3,V2,V1,V0] must be all zero.
> >>
> >> So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is
> >> EVEX.ND then simply being ignored? The ZU really is meaningful only for
> 16-bit forms, aiui ...
> >>
> >
> > EVEX.ZU should be ignored for 32-bit and 64-bit forms. For imul (in spec 6.30
> IMUL), EVEX.ND stands for ND or ZU.
> 
> In cases like this, where ignoring bits is kind of unexpected, the spec would
> better say explicitly (on the instruction page) when a meaningless bit is indeed
> ignored, rather than being reserved and causing #UD. Note how even the text
> in the APX-EVEX-INT section leaves open (or at least ambiguous, by not
> mentioning the case) whether SETcc with a memory operand ignores EVEX.ND
> or causes #UD when the bit is set.
> 

Sorry, my previous answer was inaccurate, EVEX.ZU will not be ignored in 32-bit and 64-bit forms.

Prior to Intel® APX, the following rules apply in 64-bit mode when an instruction’s destination is a GPR and
OSIZE < 64b:
1. If OSIZE is 32b, the destination GPR gets the instruction’s result in bits [31:0] and all zeros in bits
[63:32].
2. If OSIZE is 8b or 16b, the destination GPR gets the instruction’s result in bits [OSIZE-1:0] but keep its
old value in bits [63:OSIZE].

The ZU indication described in items 2.(b) of Section 3.1.2.3.1 does not introduce an NDD. For those
instructions, EVEX.ND=0 keeps the current x86 behavior, but EVEX.ND=1 forces the zeroing of bits
[63:OSIZE] for any OSIZE < 64b

> > I think ZU makes sense for both the 16-bit form (imul) and the 8-bit form
> (setcc, I'm not sure if imul supports it yet).
> 
> No, IMUL by immediate (or actually any IMUL with multiple operands) doesn't
> support byte register operands. For SETcc the ZU aspect is pretty clear and
> doesn't even need expressing by new syntax in (dis)assembly - you can simply
> distinguish the two forms by using either 8-bit registers (no ZU) or 32-/64-bit
> ones (with ZU). In principle that's possible with IMUL as well, of course, but it
> may be deemed a little odd:
> 
> 	imul	$17, %dx, %cx
> 	imul	$17, %dx, %ecx
> 
> Yet personally I'd still prefer this over adding e.g. {zu} on either the mnemonic
> or the destination operand. Question (as with the way to express
> {nf}) is how other assemblers are going to handle it. (Would be quite nice if
> the spec could at least give more clear hints towards suggested syntax, but
> that hadn't been the case already with the syntax extensions needed for
> AVX512.)
> 
 
I will add suffix “zx” (for the Intel syntax) or “zwq” (for the AT&T syntax) to the mnemonic:

Intel syntax                                              AT&T syntax
imulzx rax, word ptr[ rbx ], 0xab         imulzwq $0xab, (%rbx), %rax

Lili.
  

On 25.10.2023 17:49, Cui, Lili wrote:
>> Subject: Re: [PATCH 4/8] Support APX NDD
>>
>> On 25.10.2023 10:10, Cui, Lili wrote:
>>>> On 22.10.2023 16:05, Cui, Lili wrote:
>>>>>>> --- /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:
>>>>>>> +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.
>>>>>>
>>>>>
>>>>> We don't support it yet, I put it in RFC.
>>>>> ...
>>>>> 2. Support APX ZU   -- In progress
>>>>> 3. Support APX CCMP and CTEST -- In progress ...
>>>>>
>>>>> About 0/1 in the ND column, it means ZU can be 0/1.
>>>>>
>>>>> IMUL with opcodes 0x69 and 0x6B in map 0 and SETcc instructions
>>>>> Although these instructions do not support NDD, the EVEX.ND bit is
>>>>> used to control whether its destination register has its upper bits
>>>>> (namely,
>>>> bits [63:OSIZE]) zeroed when OSIZE is 8b or 16b.
>>>>> That is, if EVEX.ND = 1, the upper bits are always zeroed;
>>>>> otherwise, they keep the old values when OSIZE is 8b or 16b. For
>>>>> these instructions,
>>>> EVEX.[V4,V3,V2,V1,V0] must be all zero.
>>>>
>>>> So ZU indeed isn't just a typo there. For 32- and 64-bit forms, is
>>>> EVEX.ND then simply being ignored? The ZU really is meaningful only for
>> 16-bit forms, aiui ...
>>>>
>>>
>>> EVEX.ZU should be ignored for 32-bit and 64-bit forms. For imul (in spec 6.30
>> IMUL), EVEX.ND stands for ND or ZU.
>>
>> In cases like this, where ignoring bits is kind of unexpected, the spec would
>> better say explicitly (on the instruction page) when a meaningless bit is indeed
>> ignored, rather than being reserved and causing #UD. Note how even the text
>> in the APX-EVEX-INT section leaves open (or at least ambiguous, by not
>> mentioning the case) whether SETcc with a memory operand ignores EVEX.ND
>> or causes #UD when the bit is set.
>>
> 
> Sorry, my previous answer was inaccurate, EVEX.ZU will not be ignored in 32-bit and 64-bit forms.
> 
> Prior to Intel® APX, the following rules apply in 64-bit mode when an instruction’s destination is a GPR and
> OSIZE < 64b:
> 1. If OSIZE is 32b, the destination GPR gets the instruction’s result in bits [31:0] and all zeros in bits
> [63:32].
> 2. If OSIZE is 8b or 16b, the destination GPR gets the instruction’s result in bits [OSIZE-1:0] but keep its
> old value in bits [63:OSIZE].
> 
> The ZU indication described in items 2.(b) of Section 3.1.2.3.1 does not introduce an NDD. For those
> instructions, EVEX.ND=0 keeps the current x86 behavior, but EVEX.ND=1 forces the zeroing of bits
> [63:OSIZE] for any OSIZE < 64b

While described differently, that's still the same behavior as before for
OSIZE > 16b, isn't it? Which still means the EVEX.ND is effectively ignored
in those cases (and could hence as well be reserved).

>>> I think ZU makes sense for both the 16-bit form (imul) and the 8-bit form
>> (setcc, I'm not sure if imul supports it yet).
>>
>> No, IMUL by immediate (or actually any IMUL with multiple operands) doesn't
>> support byte register operands. For SETcc the ZU aspect is pretty clear and
>> doesn't even need expressing by new syntax in (dis)assembly - you can simply
>> distinguish the two forms by using either 8-bit registers (no ZU) or 32-/64-bit
>> ones (with ZU). In principle that's possible with IMUL as well, of course, but it
>> may be deemed a little odd:
>>
>> 	imul	$17, %dx, %cx
>> 	imul	$17, %dx, %ecx
>>
>> Yet personally I'd still prefer this over adding e.g. {zu} on either the mnemonic
>> or the destination operand. Question (as with the way to express
>> {nf}) is how other assemblers are going to handle it. (Would be quite nice if
>> the spec could at least give more clear hints towards suggested syntax, but
>> that hadn't been the case already with the syntax extensions needed for
>> AVX512.)
>>
>  
> I will add suffix “zx” (for the Intel syntax) or “zwq” (for the AT&T syntax) to the mnemonic:
> 
> Intel syntax                                              AT&T syntax
> imulzx rax, word ptr[ rbx ], 0xab         imulzwq $0xab, (%rbx), %rax

For Intel syntax, unless you happen to know that MASM is going to go
that route, the "word ptr" is sufficient for disambiguation, and no
suffixes should be accepted (gas) or be output (objdump). For AT&T
syntax the case with a memory operand indeed requires some means to
disambiguate; as asked before, I wonder if your approach matches with
what other assemblers are going to do.

Jan