Re: [PATCH 5/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

On 02.11.2023 12:29, 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_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 (VexVVVV_SRC): New.
> 	VexVVVV_DST):  Ditto.
> 	* opcodes/i386-opc.tbl: Add APX NDD instructions and adjust VexVVVV.
> 	* opcodes/i386-tbl.h: Regenerated.
> 
> gas/ChangeLog:
> 
> 	* gas/config/tc-i386.c (is_any_apx_evex_encoding): Add legacy insn
> 	promote to SPACE_EVEXMAP4.
> 	(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.
> 	reg_table
> 	* testsuite/gas/i386/x86-64.exp: Add x86-64-apx-ndd.
> 	* testsuite/gas/i386/x86-64-apx-ndd.d: New test.
> 	* 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.
> 	* testsuite/gas/i386/x86-64-apx-evex-promoted-bad.d : Ditto.
> 	* testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s : Ditto.
> ---
>  gas/config/tc-i386.c                          |  111 +-
>  .../gas/i386/x86-64-apx-evex-promoted-bad.d   |    4 +
>  .../gas/i386/x86-64-apx-evex-promoted-bad.s   |    5 +-
>  gas/testsuite/gas/i386/x86-64-apx-ndd.d       |  161 +++
>  gas/testsuite/gas/i386/x86-64-apx-ndd.s       |  154 +++
>  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                   |   54 +
>  opcodes/i386-dis-evex.h                       |  126 +-
>  opcodes/i386-dis.c                            |  145 +-
>  opcodes/i386-gen.c                            |    1 +
>  opcodes/i386-opc.h                            |    9 +-
>  opcodes/i386-opc.tbl                          | 1231 +++++++++--------
>  15 files changed, 1354 insertions(+), 737 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 398909a6a30..5b925505435 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2317,8 +2317,10 @@ 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_F insns just swap the two source operands,
> +	 with the 3rd one being the destination.  */
> +      if (is_cpu (t, CpuFMA4) || is_cpu (t, CpuXOP)
> +	  || is_cpu (t,CpuAPX_F))
>  	given = j < 2 ? 1 - j : j;

Nit: Please retain consistency wrt style (here: missing blank after comma
in the addition). Feels like I said so before.

> @@ -3959,6 +3961,7 @@ static INLINE bool
>  is_any_apx_evex_encoding (void)
>  {
>    return i.rex2 || i.tm.opcode_space == SPACE_EVEXMAP4 
> +    || i.rex2_encoding
>      || (i.vex.register_specifier
>  	&& i.vex.register_specifier->reg_flags & RegRex2);
>  }

See my comment on an earlier patch regarding the use of i.rex2 here. But
I doubt this is correct in the first place: Why would {rex2} cause EVEX
encoding to be picked?

> @@ -7481,26 +7484,33 @@ 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;
> +	    {
>  
> -	      case dir_encoding_store:
> -		if (!operand_type_check (operand_types[i.operands - 1], anymem)
> -		    && !t->opcode_modifier.regmem)
> -		  goto check_reverse;
> -		break;
> +	      int MemOperand = i.operands - 1 -
> +		(t->opcode_space == SPACE_EVEXMAP4
> +		 && t->opcode_modifier.vexvvvv);

Nit: I don't think local variables should start with a capital letter. I
wonder anyway - can't you just re-use e.g. j here? That would then also
avoid the misleading name: Right here you don't know yet whether that's
the "memory" operand. Finally, if you set the variable ahead if the
enclosing if(), you could (I think) avoid all this re-indentation,
making the change quite a bit easier to review (i.e. to see what really
changes).

> @@ -7530,11 +7540,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. And again in the next hunk. I won't comment
on such any further, expecting you to go through globally.

> @@ -8588,11 +8609,10 @@ 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)
> -    for (unsigned int j = 0; j < i.operands; j++)
> -      if (i.types[j].bitfield.instance != InstanceNone)
> -	i.reg_operands--;
> +     with 4 operands or less.  */
> +  for (unsigned int j = 0; j < i.operands; j++)
> +    if (i.types[j].bitfield.instance != InstanceNone)
> +      i.reg_operands--;

While you made the requested code adjustment, adjusting the comment
renders it stale now. It needs dropping instead, as it did only
explain the if(), not the for().

> @@ -8946,26 +8966,35 @@ build_modrm_byte (void)
>  				     || i.vec_encoding == vex_encoding_evex));
>      }
>  
> -  for (v = source + 1; v < dest; ++v)
> -    if (v != reg_slot)
> -      break;
> -  if (v >= dest)
> -    v = ~0;
> -  if (i.tm.extension_opcode != None)
> +  if (i.tm.opcode_modifier.vexvvvv == VexVVVV_DST)
>      {
> -      if (dest != source)
> -	v = dest;
> -      dest = ~0;
> +      v = dest;
> +      dest-- ;
>      }
> -  gas_assert (source < dest);
> -  if (i.tm.opcode_modifier.operandconstraint == SWAP_SOURCES
> -      && source != op)
> +  else if (i.tm.opcode_modifier.vexvvvv == VexVVVV_SRC)
>      {
> -      unsigned int tmp = source;
> +      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;
> +	  source = v;
> +	  v = tmp;
> +	}
>      }
> +  else
> +    v = ~0;
>  
>    if (v < MAX_OPERANDS)
>      {

I'm having trouble following this change. The VexVVVV-is-source case
shouldn't change at all, I'd expect. This would ideally be easily
visible from the change done. Yet if looking a little more closely I
can e.g. spot a stray "v = source + 1;" which wasn't there before. And
there also look to be things being dropped. Such a change (again) wants
doing such that it is easy to see what changes. If need be by making a
mechanical prereq change doing just re-indentation, but nothing else.
It feels though as if there is too much changing here anyway: The
difference for VexVVVV-is-dest is that you need to consume the
destination early. If you do so, then the rest should be possible to
keep as is: You'll subsequently deal with just the normal ModR/M, as if
without a VexVVVV-encoded operand.

> --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> @@ -10,7 +10,7 @@ _start:
>          .byte 0x62, 0xfc, 0x7f, 0x08, 0x60, 0xc2
>          .byte 0xff, 0xff
>          #VSIB vpgatherqq 0x7b(%rbp,%zmm17,8),%zmm16{%k1} set EVEX.P[10] == 0
> -	#(illegal value).
> +        #(illegal value).
>          .byte 0x62, 0xe2, 0xf9, 0x41, 0x91, 0x84, 0xcd, 0x7b, 0x00, 0x00, 0x00
>          .byte 0xff
>          #EVEX_MAP4 movbe %r18w,%ax set EVEX.mm == b01 (illegal value).

???

> @@ -27,3 +27,6 @@ _start:
>          .byte 0xff
>          #EVEX from VEX enqcmd 0x123(%r31,%rax,4),%r31 EVEX.P[23:22] == 1 (illegal value).
>          .byte 0x62, 0x4c, 0x7f, 0x28, 0xf8, 0xbc, 0x87, 0x23, 0x01, 0x00, 0x00
> +        .byte 0xff

This then similarly looks to belong into the earlier patch, suggesting
that it had #pass too early.

> +        #{evex} inc %rax EVEX.vvvv' > 0 (illegal value).
> +        .byte 0x62, 0xf4, 0xec, 0x08, 0xff, 0xc0

Again I suspect this can be expressed via .insn, thus ending up more clear.
I can only stress again that one of the reasons of introducing .insn was to
reduce the number of such entirely unreadable byte sequences.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd.s
> @@ -0,0 +1,154 @@
> +# Check 64bit APX NDD instructions with evex prefix encoding
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +inc    %rax,%rbx

Please can instructions be indented by a tab?

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

While there's a doc problem here as well, the question here and there is
the same: Which form of ROR does this represent, when the shift count
isn't specified explicitly? It could be %cl or $1. I would strongly
advise against introducing further ambiguous instruction patterns like
this, and instead demand that both %cl and $1 be always named explicitly
as operands.

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

Overall there's also again the sorting criteria question: Without any
sorting, how is one to (easily) check full coverage?

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -473,6 +473,7 @@ static bitfield opcode_modifiers[] =
>    BITFIELD (IntelSyntax),
>    BITFIELD (ISA64),
>    BITFIELD (NoEgpr),
> +  BITFIELD (NF),
>  };

This wants introducing earlier, when the BMI / BMI2 templates are touched
anyway. As said before, it would be very nice if within such a series the
same places wouldn't needlessly be touched more than once. You don't
implement parsing of NF here anyway, so how much in advance the attribute
is added to the opcode table is really irrelevant, and should hence be
done as conventiently as possible.

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -636,7 +636,10 @@ enum
>    /* 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 VexVVVV_SRC   1
> +#define VexVVVV_DST   2
>    VexVVVV,

Nit: Singular in the new comment line, please (plus perhaps drop "as").
And "source" wants adding to the earlier comment line.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -138,6 +138,9 @@
>  #define Vsz512 Vsz=VSZ512
>  
>  #define APX_F APX_F|x64
> +#define VexVVVVSrc  VexVVVV=VexVVVV_SRC
> +#define VexVVVVDest VexVVVV=VexVVVV_DST

I don't this we need the former. Continuing to use just VexVVVV there
is going to be quite fine, and easier to read. Plus, I'm sorry to say
it this bluntly, it is entirely inappropriate to bloat an already
large patch by mechanically replacing VexVVVV by this new alias. If
such a change was really wanted, it would need separating out as an
entirely mechanical one.

For the latter, to help readability, how about DstVVVV?

> +
>  
>  // The EVEX purpose of StaticRounding appears only together with SAE. Re-use
>  // the bit to mark commutative VEX encodings where swapping the source

Please don't add stray (especially double) blank lines. Instead a
blank line would be wanted _ahead_ of the addition.

> @@ -190,6 +193,8 @@ mov, 0xf21, i386|No64, D|RegMem|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_qSuf, { De
>  mov, 0xf21, x64, D|RegMem|No_bSuf|No_wSuf|No_lSuf|No_sSuf|NoRex64, { Debug, Reg64 }
>  mov, 0xf24, i386|No64, D|RegMem|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|No_qSuf, { Test, Reg32 }
>  
> +// Move after swapping the bytes
> +movbe, 0x0f38f0, Movbe, D|Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Word|Dword|Qword|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
>  // Move after swapping the bytes
>  movbe, 0x0f38f0, Movbe, D|Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Word|Dword|Qword|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
>  movbe, 0x60, Movbe|APX_F, D|Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVex128|EVexMap4, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }

What is this addition about? Please can you look at your own patches before
sending them out?

> @@ -290,22 +295,36 @@ add, 0x0, 0, D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, { Reg8|Reg16|Reg3
>  add, 0x83/0, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S, Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex }
>  add, 0x4, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S, Acc|Byte|Word|Dword|Qword }
>  add, 0x80/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex }
> +add, 0x0, APX_F, D|W|CheckOperandSize|Modrm|No_sSuf|VexVVVVDest|EVex128|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64 }
> +add, 0x83/0, APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|VexVVVVDest|EVex128|EVexMap4|NF, { Imm8S, Reg16|Reg32|Reg64|Word|Dword|Qword|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> +add, 0x80/0, APX_F, W|Modrm|CheckOperandSize|No_sSuf|VexVVVVDest|EVex128|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64}

Earlier review comments were not addressed. I'll stop here, as far as this
file is concerned, expecting a new version to be sent addressing earlier
comments and having been sanity checked and having the bogus VexVVVV
renaming dropped.

Jan