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

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

Thanks for your reviews, I have responded to your comments. The new patch will be attached to a subsequent email.

BRs,
Lin

-----Original Message-----
From: Jan Beulich <jbeulich@suse.com> 
Sent: Thursday, September 28, 2023 5:30 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 5/8] Support APX NDD optimized encoding.

On 19.09.2023 17:25, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -7091,6 +7091,46 @@ check_EgprOperands (const insn_template *t)
>    return 0;
>  }
>  
> +/* Optimize APX NDD insns to non-NDD insns.  */
> +
> +static int

"bool" please when the function merely returns a yes/no indicator.

* Have modified.

> +optimize_NDD_to_nonNDD (const insn_template *t) {
> +  if (t->opcode_modifier.vexvvvv
> +      && t->opcode_space == SPACE_EVEXMAP4
> +      && i.reg_operands >= 2

See the remark near the bottom of the changes to this file: This condition is likely insufficient, as
- further insns allowing ND may not be treated this way (CCMPscc,
  CTESTscc, and one of the CFCMOVcc forms at the very least),
- {nf} uses will want excluding, as it would be merely a waste of
  time to try to re-match with fewer operands.

* CCMPSCC and CTESTSCC’s vexvvvv will be false. I think one of the CFCMOVCC forms is same.

> +      && (i.types[i.operands - 1].bitfield.dword
> +	  || i.types[i.operands - 1].bitfield.qword))

Why do you exclude byte and word operations? Imo what you want to check is class being Reg.

* It looks like I misunderstood the requirements, I've removed it.

> +    {
> +      int tmp_flag = -1;

Either type or name need to change: A variable of this name wants to be "bool". I would have suggested "dupl" as the name, but that how doesn't fit how the variable is used below.

(Of course I'm also not happy with the use of plain int here and below, but that's a wider issue throughout the source file. Still it would be nice if new code properly used unsigned int whenever only non-negative values are to be held.)

* Have changed var name and type.

> +      int dest = i.operands - 1;
> +      int src1 = (i.operands > 2) ? i.operands - 2 : 0;
> +      int src2 = (i.operands > 3) ? i.operands - 3 : 0;
> +
> +      if (i.op[src1].regs == i.op[dest].regs)
> +	tmp_flag = src2;
> +      /* adcx and adox don't have D bit.  */

IMUL doesn't either, yet ought to also be eligible? We have a "commutative"
flag already - can't you extend its use to the purposes you have here? I expect this would simplify ...

> +      else if (i.op[src2].regs == i.op[dest].regs

Just to mention it: Both this and the earlier similar equality check aren't entirely legal, as you haven't previously checked that the respective operands actually are register ones. Analysis tools (like the UB
sanitizer) may choke on this, even if otherwise this shouldn't be a problem from what I can tell.

* Type checking has been added.

> +	       && (t->opcode_modifier.d
> +		   || t->mnem_off == MN_adcx
> +		   || t->mnem_off == MN_adox)
> +	       && (t->mnem_off != MN_sub)
> +	       && (t->mnem_off != MN_sbb))

... this condition.

* I didn't notice the imul, already added it. Have used “commutative” to mark insns that can swap source operands for optimization.

> +	tmp_flag = src1;
> +      if (tmp_flag != -1)
> +	{
> +	  --i.operands;
> +	  --i.reg_operands;
> +	  --i.tm.operands;
> +
> +	  if (tmp_flag != src2)
> +	      swap_2_operands (tmp_flag, src2);

Nit: There's once again something wrong with indentation here.

> +	  return 1;
> +	}
> +    }
> +  return 0;
> +}
> +
>  /* Helper function for the progress() macro in match_template().  */  
> static INLINE enum i386_error progress (enum i386_error new,
>  					enum i386_error last,
> @@ -7562,6 +7602,15 @@ match_template (char mnem_suffix)
>  	     slip through to break.  */
>  	}
>  
> +      /* If we can optimize a NDD insn to non-NDD insn, like
> +	 add %r16, %r8, %r8 -> add %r16, %r8, then rematch template.  */
> +      if (optimize_NDD_to_nonNDD (t))

I don't think such an optimization should be done without any form of -O.

As to the function name, maybe better optimize_NDD_to_REX2()?

* Refer to the optimization of VOP, temporarily set to O1 will be optimized.  If we use 32bit register, some instructions will be optimized from NDD to rex or legacy. Like cmovg  0x90909090(%eax),%edx,%edx, imul   %rdx,%rax,%rdx in our test.

> +	{
> +	  t = current_templates->start;
> +	  --t;
> +	  continue;

So the decrement is to compensate the loop continuation. Nevertheless imo this wants spelling "t = current_templates->start - 1". Yet like above note that a good UB checker may object to this subtraction of 1, unless you build upon templates making it here not being part of the first group in i386_optab[]. (If any such dependency exists, it needs spelling out in a suitable place, i.e. in particular one that's hard to overlook when doing re-arrangements.)

* I think it’s ok at the moment, because the first group in i386_optab is a legacy mov. I will add a comment in the i386_opc.tbl.

> +	}
> +
>        /* Check if VEX/EVEX encoding requirements can be satisfied.  */
>        if (VEX_check_encoding (t))
>  	{

I also wonder whether this doesn't need moving further down. I expect at least VEX_check_encoding() may need running first.

But I'm worried anyway that this patch comes too early in the series.
{evex} prefixes or use of {nf} would need skipping the optimization attempt, and getting all the conditions right is likely easier to see when all the rest of the infrastructure is in place.

* Have reordered.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.s
> @@ -0,0 +1,115 @@
> +# Check 64bit APX NDD instructions with optimized encoding
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +inc    %ax,%ax
> +inc    %eax,%eax
> +inc    %rax,%rax
> +inc    %r16,%r16
> +add    %r31b,%r8b,%r8b
> +add    %r31b,%r8b,%r31b
> +addb    %r31b,%r8b,%r8b
> +add    %r31,%r8,%r8
> +addq    %r31,%r8,%r8
> +add    %r31d,%r8d,%r8d
> +addl    %r31d,%r8d,%r8d
> +add    %r31w,%r8w,%r8w
> +addw    %r31w,%r8w,%r8w
> +{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
> +dec    %r17,%r17
> +not    %r17,%r17
> +neg    %r17,%r17
> +sub    %r15,%r17,%r17
> +sub    %r15d,(%r8),%r15d
> +sub    (%r15,%rax,1),%r16,%r16
> +sub    $0x1234,%r30,%r30
> +sbb    %r15,%r17,%r17
> +sbb    %r15,(%r8),%r15
> +sbb    (%r15,%rax,1),%r16,%r16
> +sbb    $0x1234,%r30,%r30
> +adc    %r15,%r17,%r17
> +adc    %r15d,(%r8),%r15d
> +adc    (%r15,%rax,1),%r16,%r16
> +adc    $0x1234,%r30,%r30
> +or    %r15,%r17,%r17
> +or    %r15d,(%r8),%r15d
> +or    (%r15,%rax,1),%r16,%r16
> +or    $0x1234,%r30,%r30
> +xor    %r15,%r17,%r17
> +xor    %r15d,(%r8),%r15d
> +xor    (%r15,%rax,1),%r16,%r16
> +xor    $0x1234,%r30,%r30
> +and    %r15,%r17,%r17
> +and    %r15d,(%r8),%r15d
> +and    (%r15,%rax,1),%r16,%r16
> +and    $0x1234,%r30,%r30
> +and    $0x1234,%r30
> +ror    %r31,%r31
> +rorb   %r31b,%r31b

Please be consistent with omitting (or having) suffixes (further up you simply test both variants, but that's not the case throughout ...

> +ror    $0x2,%r12,%r12
> +rol    %r31,%r31
> +rolb   %r31b,%r31b
> +rol    $0x2,%r12,%r12
> +rcr    %r31,%r31
> +rcrb   %r31b,%r31b
> +rcr    $0x2,%r12b,%r12b
> +rcr    $0x2,%r12,%r12
> +rcl    %r31,%r31
> +rclb   %r31b,%r31b
> +rcl    $0x2,%r12b,%r12b
> +rcl    $0x2,%r12,%r12
> +shl    %r31,%r31
> +shlb   %r31b,%r31b
> +shl    $0x2,%r12b,%r12b
> +shl    $0x2,%r12,%r12
> +sar    %r31,%r31
> +sarb   %r31b,%r31b
> +sar    $0x2,%r12b,%r12b
> +sar    $0x2,%r12,%r12
> +shl    %r31,%r31
> +shlb   %r31b,%r31b
> +shl    $0x2,%r12b,%r12b
> +shl    $0x2,%r12,%r12
> +shr    %r31,%r31
> +shrb   %r31b,%r31b

... here.

* Added suffixes to the command names, I don't understand very well what is suggested inside the parentheses, I just aligned most of the insns' test items.

Jan