RE: [PATCH v3 8/9] Support APX NDD optimized encoding.

["Hu, Lin1" <lin1.hu@intel.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, December 11, 2023 8:28 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; Hu, Lin1 <lin1.hu@intel.com>;
> binutils@sourceware.org
> Subject: Re: [PATCH v3 8/9] Support APX NDD optimized encoding.
> 
> On 24.11.2023 08:02, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -7148,6 +7148,58 @@ check_APX_operands (const insn_template *t)
> >    return 0;
> >  }
> >
> > +/* Check if the instruction use the REX registers.  */ static bool
> > +check_RexOperands () {
> > +  for (unsigned int op = 0; op < i.operands; op++)
> > +    {
> > +      if (i.types[op].bitfield.class != Reg)
> > +	continue;
> > +
> > +      if (i.op[op].regs->reg_flags & (RegRex | RegRex64))
> > +	return true;
> > +    }
> > +
> > +  if ((i.index_reg && (i.index_reg->reg_flags & (RegRex | RegRex64)))
> > +      || (i.base_reg && (i.base_reg->reg_flags & (RegRex | RegRex64))))
> > +    return true;
> > +
> > +  /* Check pseudo prefix {rex} are valid.  */  return i.rex_encoding;
> 
> Can this actually happen, when we're converting from EVEX to legacy?
> (Initially I wanted to ask about "rex" and alike prefixes, i.e. the non- pseudo
> ones.)
>

This is to align with check_EgprOperands. I hope the function be more general. Not just for this optimization problem.
 
>
> > +}
> > +
> > +/* Optimize APX NDD insns to legacy insns.  */ static unsigned int
> > +can_convert_NDD_to_legacy (const insn_template *t) {
> > +  unsigned int match_dest_op = ~0;
> > +
> > +  if (t->opcode_modifier.vexvvvv == VexVVVV_DST
> 
> No new callers are expected to appear (any time soon) and the sole caller has
> checked this already.
>

I will remove the condition.
 
>
> Also with this check, ...
> 
> > +      && t->opcode_space == SPACE_EVEXMAP4
> 
> ... what (further) effect is this one intended to have?
>

At first it was because of map4+vexvvvvv in order to locate the instructions that needed to be optimized. It seems that it's useless now. I will remove it.
 
>
> > +      && !i.has_nf
> > +      && i.reg_operands >= 2)
> > +    {
> > +      unsigned int dest = i.operands - 1;
> > +      unsigned int src1 = i.operands - 2;
> > +      unsigned int src2 = (i.operands > 3) ? i.operands - 3 : 0;
> > +
> > +      if (i.types[src1].bitfield.class == Reg
> > +	  && i.op[src1].regs == i.op[dest].regs)
> > +	match_dest_op = src1;
> > +      /* If the first operand is the same as the third operand,
> > +	 these instructions need to support the ability to commutative
> > +	 the first two operands and still not change the semantics in order
> > +	 to be optimized.  */
> > +      else if (i.types[src2].bitfield.class == Reg
> > +	       && i.op[src2].regs == i.op[dest].regs
> > +	       && optimize > 1
> > +	       && t->opcode_modifier.commutative)
> 
> Based on the "cheap conditions first" principle and to also be better in line with
> the comment, may I suggest
> 
> +      else if (optimize > 1
> +	       && t->opcode_modifier.commutative
> +	       && i.types[src2].bitfield.class == Reg
> +	       && i.op[src2].regs == i.op[dest].regs)
> 
> ?
>

Thanks your advice, I have modified.

> 
> > +	match_dest_op = src2;
> > +    }
> > +  return match_dest_op;
> > +}
> > +
> >  /* Helper function for the progress() macro in match_template().  */
> > static INLINE enum i386_error progress (enum i386_error new,
> >  					enum i386_error last,
> > @@ -7675,6 +7727,61 @@ match_template (char mnem_suffix)
> >  	  i.memshift = memshift;
> >  	}
> >
> > +      /* If we can optimize a NDD insn to legacy insn, like
> > +	 add %r16, %r8, %r8 -> add %r16, %r8,
> > +	 add  %r8, %r16, %r8 -> add %r16, %r8, then rematch template.
> > +	 Note that the semantics have not been changed.  */
> > +      if (optimize
> > +	  && !i.no_optimize
> > +	  && i.vec_encoding != vex_encoding_evex
> > +	  && t + 1 < current_templates->end
> > +	  && !t[1].opcode_modifier.evex
> > +	  && t[1].opcode_space <= SPACE_0F38
> > +	  && t->opcode_modifier.vexvvvv == VexVVVV_DST)
> > +	{
> > +	  unsigned int match_dest_op = can_convert_NDD_to_legacy (t);
> > +	  size_match = true;
> 
> This would perhaps better ...
> 
> > +	  if (match_dest_op != (unsigned int) ~0)
> > +	    {
> 
> ... live here
>

OK.
 
>
> > +	      /* We ensure that the next template has the same input
> > +		 operands as the original matching template by the first
> > +		 opernd (ATT), thus avoiding the error caused by the wrong
> order
> > +		 of insns in i386.tbl.  */
> 
> I'm sorry, but I (still) can't make sense of this last part of the comment, after the
> comma.
>

I mean if someone support new NDD insns and put it in the wrong position, so the part will try to avoid to optimize the insn.
 
>
> > +	      overlap0 = operand_type_and (i.types[0],
> > +					   t[1].operand_types[0]);
> > +	      if (t->opcode_modifier.d)
> > +		overlap1 = operand_type_and (i.types[0],
> > +					     t[1].operand_types[1]);
> > +	      if (!operand_type_match (overlap0, i.types[0])
> > +		  && (!t->opcode_modifier.d
> > +		      || (t->opcode_modifier.d
> > +			  && !operand_type_match (overlap1, i.types[0]))))
> 
> What's wrong with the simpler
> 
> 		  && (!t->opcode_modifier.d
> 		      || !operand_type_match (overlap1, i.types[0])))
> 
> ?
>

Yes, the simpler is useful, I have modified these conditions.
 
>
> > +		size_match = false;
> 
> Yet still, and despite the improved comment, I don't really see what all of this is
> about. What cases would be mis-handled if this wasn't there?
>

Not currently. It's for someone support new NDD instructions with wrong order. My idea is to prefer not to optimize, but try to avoid reporting error.
 
>
> > +	      if (size_match
> > +		  /* Optimizing some non-legacy-map0/1 without REX/REX2
> prefix will be valuable.  */
> > +		  && (t[1].opcode_space <= SPACE_0F
> 
> Where a comment is placed is meaningful to understanding what it's about. The
> wayy you have it, is says "non-legacy-map0/1" on a check that the (next)
> encoding is map0 or map1. I think this wants moving down by a line, and even
> then also re-wording: If I didn't (vaguely) recall context, I don't think I could
> derive what is meant. Iirc this is about legacy encodings being one byte shorter
> for certain 0f38 space insns when they don't require a REX prefix to encode.
> How about something like "Some non-legacy-map0/1 insns can be shorter when
> legacy-encoded and when no REX prefix is required"?
>

OK, I have modified. 
 
>
> > +		      || (!check_EgprOperands (t + 1)
> > +			  && !check_RexOperands ()
> 
> I'm not going to insist that you adjust this, but these two calls side by side
> demonstrate a curious inconsistency: The former requires t to be passed in. If
> you keep it like that, I may change this down the road, the more that the t-
> related aspect isn't relevant here at all (and could hence be moved out of the
> function to the single place where it is needed).
>

I will keep the part of code. I prefer to modified check_EgprOperands in another patch.
 
>
> > +			  && !i.op[i.operands - 1].regs-
> >reg_type.bitfield.qword)))
> > +		{
> > +		  unsigned int src1 = i.operands - 2;
> > +		  unsigned int src2 = (i.operands > 3) ? i.operands - 3 : 0;
> > +
> > +		  if (match_dest_op == src2)
> > +		    swap_2_operands (match_dest_op, src1);
> 
> Isn't it wrong (albeit benign) to swap when i.operands == 2? IOW wouldn't
> 
> 		  if (i.reg_operands > 2 && match_dest_op == i.operands - 3)
> 		    swap_2_operands (match_dest_op, i.operands - 2);
> 
> be more in line with what's actually wanted?
> 

Because some insn use memory (like add %r8, (%rsp), %r8), so I change the code be
		  if (i.operands > 2 && match_dest_op == i.operands - 3)
		    swap_2_operands (match_dest_op, i.operands - 2);

>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.s
> > @@ -0,0 +1,123 @@
> > +# Check 64bit APX NDD instructions with optimized encoding
> > +
> > +	.text
> > +_start:
> > +add    %r31,%r8,%r8
> > +addb   %r31b,%r8b,%r8b
> > +{store} add    %r31,%r8,%r8
> > +{load}  add    %r31,%r8,%r8
> > +add    %r31,(%r8),%r31
> > +add    (%r31),%r8,%r8
> > +add    $0x12344433,%r15,%r15
> > +add    $0xfffffffff4332211,%r8,%r8
> > +inc    %r31,%r31
> > +incb   %r31b,%r31b
> > +sub    %r15,%r17,%r17
> > +subb   %r15b,%r17b,%r17b
> > +sub    %r15,(%r8),%r15
> > +sub    (%r15,%rax,1),%r16,%r16
> > +sub    $0x1234,%r30,%r30
> > +dec    %r17,%r17
> > +decb   %r17b,%r17b
> > +sbb    %r15,%r17,%r17
> > +sbbb   %r15b,%r17b,%r17b
> > +sbb    %r15,(%r8),%r15
> > +sbb    (%r15,%rax,1),%r16,%r16
> > +sbb    $0x1234,%r30,%r30
> > +and    %r15,%r17,%r17
> > +andb   %r15b,%r17b,%r17b
> > +and    %r15,(%r8),%r15
> > +and    (%r15,%rax,1),%r16,%r16
> > +and    $0x1234,%r30,%r30
> > +or     %r15,%r17,%r17
> > +orb    %r15b,%r17b,%r17b
> > +or     %r15,(%r8),%r15
> > +or     (%r15,%rax,1),%r16,%r16
> > +or     $0x1234,%r30,%r30
> > +xor    %r15,%r17,%r17
> > +xorb   %r15b,%r17b,%r17b
> > +xor    %r15,(%r8),%r15
> > +xor    (%r15,%rax,1),%r16,%r16
> > +xor    $0x1234,%r30,%r30
> > +adc    %r15,%r17,%r17
> > +adcb   %r15b,%r17b,%r17b
> > +adc    %r15,(%r8),%r15
> > +adc    (%r15,%rax,1),%r16,%r16
> > +adc    $0x1234,%r30,%r30
> > +neg    %r17,%r17
> > +negb   %r17b,%r17b
> > +not    %r17,%r17
> > +notb   %r17b,%r17b
> > +imul   0x90909(%eax),%edx,%edx
> > +imul   0x909(%rax,%r31,8),%rdx,%rdx
> > +imul   %rdx,%rax,%rdx
> > +rol    %r31,%r31
> > +rolb   %r31b,%r31b
> > +rol    $0x2,%r12,%r12
> > +rolb   $0x2,%r12b,%r12b
> > +ror    %r31,%r31
> > +rorb   %r31b,%r31b
> > +ror    $0x2,%r12,%r12
> > +rorb   $0x2,%r12b,%r12b
> > +rcl    %r31,%r31
> > +rclb   %r31b,%r31b
> > +rcl    $0x2,%r12,%r12
> > +rclb   $0x2,%r12b,%r12b
> > +rcr    %r31,%r31
> > +rcrb   %r31b,%r31b
> > +rcr    $0x2,%r12,%r12
> > +rcrb   $0x2,%r12b,%r12b
> > +sal    %r31,%r31
> > +salb   %r31b,%r31b
> > +sal    $0x2,%r12,%r12
> > +salb   $0x2,%r12b,%r12b
> > +shl    %r31,%r31
> > +shlb   %r31b,%r31b
> > +shl    $0x2,%r12,%r12
> > +shlb   $0x2,%r12b,%r12b
> > +shr    %r31,%r31
> > +shrb   %r31b,%r31b
> > +shr    $0x2,%r12,%r12
> > +shrb   $0x2,%r12b,%r12b
> > +sar    %r31,%r31
> > +sarb   %r31b,%r31b
> > +sar    $0x2,%r12,%r12
> > +sarb   $0x2,%r12b,%r12b
> > +shld   $0x1,%r12,(%rax),%r12
> > +shld   $0x2,%r8,%r12,%r12
> > +shld   $0x2,%r8,%r12,%r8
> > +shld   %cl,%r9,(%rax),%r9
> > +shld   %cl,%r12,%r16,%r16
> > +shld   %cl,%r12,%r16,%r12
> > +shrd   $0x1,%r12,(%rax),%r12
> > +shrd   $0x1,%r13,%r12,%r12
> > +shrd   $0x1,%r13,%r12,%r13
> > +shrd   %cl,%r9,(%rax),%r9
> > +shrd   %cl,%r12,%r16,%r16
> > +shrd   %cl,%r12,%r16,%r12
> > +cmovo  0x90909090(%eax),%edx,%edx
> > +cmovno 0x90909090(%eax),%edx,%edx
> > +cmovb  0x90909090(%eax),%edx,%edx
> > +cmovae 0x90909090(%eax),%edx,%edx
> > +cmove  0x90909090(%eax),%edx,%edx
> > +cmovne 0x90909090(%eax),%edx,%edx
> > +cmovbe 0x90909090(%eax),%edx,%edx
> > +cmova  0x90909090(%eax),%edx,%edx
> > +cmovs  0x90909090(%eax),%edx,%edx
> > +cmovns 0x90909090(%eax),%edx,%edx
> > +cmovp  0x90909090(%eax),%edx,%edx
> > +cmovnp 0x90909090(%eax),%edx,%edx
> > +cmovl  0x90909090(%eax),%edx,%edx
> > +cmovge 0x90909090(%eax),%edx,%edx
> > +cmovle 0x90909090(%eax),%edx,%edx
> > +cmovg  0x90909090(%eax),%edx,%edx
> > +adcx   %ebx,%eax,%eax
> > +adcx   %eax,%ebx,%eax
> > +adcx   %rbx,%rax,%rax
> > +adcx   %r15,%r8,%r8
> 
> Might this better be
> 
> adcx   %r15d,%r8d,%r8d
> 
> to avoid having two exclusion criteria (REX register use and REX.W set)?
> Or maybe even split to further separate source and destination:
> 
> adcx   %eax,%r8d,%r8d
> adcx   %r15d,%eax,%eax
> 
> ?
>

OK, I have split adcx and adox.
 
BR,
Lin