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

[Jan Beulich <jbeulich@suse.com>]

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.

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

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

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

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

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

> +	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()?

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

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

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

Jan