RE: [PATCH 5/8] Support APX NDD

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

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

Done.

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

Deleted.

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

Good idea. Changed.

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

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

Done.

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

Changed to add these codes before the old code.

  if (i.tm.opcode_modifier.vexvvvv == VexVVVV_DST)
    {
      v = dest;
      dest-- ;
    }
  else
    {
...
    }

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

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

Done.

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

Done.

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

Done.  GCC also changed.

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

Done.

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

Done.

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

Removed VexVVVV_SRC.

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

Done.

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

Done.

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

Done.

> > @@ -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|BaseInde
> x }
> > +add, 0x0, APX_F,
> >
> +D|W|CheckOperandSize|Modrm|No_sSuf|VexVVVVDest|EVex128|EVexMa
> p4|NF, {
> > +Reg8|Reg16|Reg32|Reg64,
> >
> +Reg8|Reg16|Reg32|Reg64|Byte|Word|Dword|Qword|Unspecified|BaseInd
> ex,
> > +Reg8|Reg16|Reg32|Reg64 } add, 0x83/0, APX_F,
> >
> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|VexVVVVDest|EVex128|EVex
> Map4|N
> > +F, { 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|BaseInd
> ex,
> > +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.
> 

 Ok.

Thanks,
Lili.