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

[Jan Beulich <jbeulich@suse.com>]

On 02.11.2023 12:29, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -7208,6 +7208,44 @@ check_EgprOperands (const insn_template *t)
>    return 0;
>  }
>  
> +/* Optimize APX NDD insns to non-NDD insns.  */
> +
> +static bool
> +optimize_NDD_to_nonNDD (const insn_template *t)
> +{
> +  if (t->opcode_modifier.vexvvvv == VexVVVV_DST
> +      && t->opcode_space == SPACE_EVEXMAP4
> +      && !i.has_nf

As mentioned before, I'd still prefer the optimization to only be
added after NF handling was put in place. But I'm not meaning to make
this a strict requirement: Introducing the has_nf field here (with
it only being read, never [explicitly] written) is certainly okay-ish.
(But see also below.)

Similarly I'm concerned of the ND form of CFCMOVcc, which isn't there
yet in the patches, but which will also need excluding from this
optimization. Obviously this concern then extends to any future ND-
encoded insns, which (likely) won't have legacy-encoded (and hence
REX2-encodable) counterparts. Are there any plans how to deal with
such? (There's a possible approach mentioned further down.)

> +      && i.reg_operands >= 2
> +      && i.types[i.operands - 1].bitfield.class == Reg)

Isn't this implicit from the VexVVVV check further up?

> +    {
> +      unsigned int readonly_var = ~0;
> +      unsigned int dest = i.operands - 1;
> +      unsigned int src1 = (i.operands > 2) ? i.operands - 2 : 0;

Since we already know i.operands >= 2 from the earlier check of
i.reg_operands, can't this simply be

      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)
> +	readonly_var = src2;

As can be seen in the testcase, this also results in ADCX/ADOX to be
converted to non-ND EVEX forms, i.e. even when that's not a win at all.
We shouldn't change what the user has written when the encoding doesn't
actually improve. (Or else, but I'd be hesitant to accept that, at the
very least the effect would need pointing out in the description or
even a code comment, so that later on it is possible to figure out
whether that was intentional or an oversight.)

This is where my template ordering remark in reply to patch 5 comes
into play: Whether invoking re-parse is okay would further need to
depend on whether an alternative (earlier) template actually allows
REX2 encoding (same base-opcode could be one of the criteria for how
far to look back through earlier templates; an option might also be to
put the 3-operand templates first, so that looking backwards wouldn't
be necessary in the first place). This would then likely also address
one of the forward looking concerns I've raised above.

> +      /* adcx, adox and imul don't have D bit.  */
> +      else if (i.types[src2].bitfield.class == Reg
> +	       && i.op[src2].regs == i.op[dest].regs
> +	       && t->opcode_modifier.commutative)

There's a disconnect between comment and code here: You don't use the
D attribute, so why is it being mentioned?

> +	readonly_var = src1;
> +      if (readonly_var != (unsigned int) ~0)
> +	{
> +	  --i.operands;
> +	  --i.reg_operands;
> +	  --i.tm.operands;
> +
> +	  if (readonly_var != src2)
> +	    swap_2_operands (readonly_var, src2);

May I suggest that just out of precaution the swapping be done before
operand counts are decremented? In principle swap_2_operands() could
do with having assertions added as to it actually dealing with valid
operands. (You'll note that elsewhere, when we add a new operand, we
increment first and then swap.)

> +	  return 1;
> +	}
> +    }
> +  return 0;
> +}

Nit: A function returning bool would preferably return true/false, not
0/1.

> @@ -7728,6 +7766,14 @@ match_template (char mnem_suffix)
>  	  i.memshift = memshift;
>  	}
>  
> +      /* If we can optimize a NDD insn to non-NDD insn, like
> +	 add %r16, %r8, %r8 -> add %r16, %r8, then rematch template.  */
> +      if (optimize == 1 && optimize_NDD_to_nonNDD (t))

So you do this optimization at -O1, but not at -O2? Imo the "== 1"
simply needs dropping. Furthermore the {nooptimize} and {evex} pseudo
prefixes need respecting. Quite likely respecting {evex} would
eliminate the need for the explicit .has_nf check in the helper
function, as I expect .vec_encoding to be set alongside that bit
anyway. Further quite likely respecting {evex} here will mean that in
patch 3 you need to introduce a new enumerator (e.g.
vex_encoding_egpr, vaguely similar to vex_encoding_evex512), to avoid
setting .vec_encoding to vex_encoding_evex when an eGPR is parsed.

As to optimization level: In build_vex_prefix() we leverage C only
at -O2 or higher (including -Os). We may want to be consistent in this
regard here (i.e. by an extra check in the helper function).

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

As per a remark further up, this adjustment could be avoided if the ND
templates came ahead of the legacy ones. They can't be wrongly used in
place of the legacy ones, due to the extra operand they require. Then
a comment here would merely point out this ordering aspect. But of
course care will then need to be taken to not go past i386_optab[]'s
bounds (by having suitably ordered conditionals when looking for
whether there is an alternative template in the first place; again see
the respective remark further up).

> +	  continue;
> +	}

Btw, considering this re-matching, I wonder whether "convert" wouldn't
be better in the function name compared to "optimize".

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.s
> @@ -0,0 +1,117 @@
> +# Check 64bit APX NDD instructions with optimized encoding
> +
> +	.text
> +_start:
> +inc    %r31,%r31
> +incb   %r31b,%r31b
> +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
> +dec    %r17,%r17
> +decb   %r17b,%r17b
> +not    %r17,%r17
> +notb   %r17b,%r17b
> +neg    %r17,%r17
> +negb   %r17b,%r17b
> +sub    %r15,%r17,%r17
> +subb   %r15b,%r17b,%r17b
> +sub    %r15,(%r8),%r15
> +sub    (%r15,%rax,1),%r16,%r16
> +sub    $0x1234,%r30,%r30
> +sbb    %r15,%r17,%r17
> +sbbb   %r15b,%r17b,%r17b
> +sbb    %r15,(%r8),%r15
> +sbb    (%r15,%rax,1),%r16,%r16
> +sbb    $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
> +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
> +and    %r15,%r17,%r17
> +andb   %r15b,%r17b,%r17b
> +and    %r15,(%r8),%r15
> +and    (%r15,%rax,1),%r16,%r16
> +and    $0x1234,%r30,%r30
> +ror    %r31,%r31
> +rorb   %r31b,%r31b
> +ror    $0x2,%r12,%r12
> +rorb   $0x2,%r12b,%r12b
> +rol    %r31,%r31
> +rolb   %r31b,%r31b
> +rol    $0x2,%r12,%r12
> +rolb   $0x2,%r12b,%r12b
> +rcr    %r31,%r31
> +rcrb   %r31b,%r31b
> +rcr    $0x2,%r12,%r12
> +rcrb   $0x2,%r12b,%r12b
> +rcl    %r31,%r31
> +rclb   %r31b,%r31b
> +rcl    $0x2,%r12,%r12
> +rclb   $0x2,%r12b,%r12b
> +shl    %r31,%r31
> +shlb   %r31b,%r31b
> +shl    $0x2,%r12,%r12
> +shlb   $0x2,%r12b,%r12b
> +sar    %r31,%r31
> +sarb   %r31b,%r31b
> +sar    $0x2,%r12,%r12
> +sarb   $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
> +shld   $0x1,%r12,(%rax),%r12
> +shld   $0x2,%r8,%r12,%r12
> +shld   %cl,%r9,(%rax),%r9
> +shld   %cl,%r12,%r16,%r16
> +shld   %cl,%r13,(%r19,%rax,4),%r13

What's the difference (in what is being tested) between this and the
first of the %cl tests? Shouldn't one of them rather be of the form
"reg1,reg2,reg1"? And then shouldn't there be a similar test with an
immediate operand? (Same for SHRD then, obviously.)

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -145,6 +145,8 @@
>  // The EVEX purpose of StaticRounding appears only together with SAE. Re-use
>  // the bit to mark commutative VEX encodings where swapping the source
>  // operands may allow to switch from 3-byte to 2-byte VEX encoding.
> +// And re-use the bit to mark some NDD insns that swapping the source operands
> +// may allow to switch from 3 operands to 2 operands.
>  #define C StaticRounding

The 3-to-2 conversion isn't what we're primarily after (see comments above).
It's the EVEX->REX2 encoding conversion which we'd like to do.

> @@ -166,6 +168,10 @@
>  
>  ### MARKER ###
>  
> +// Please don't add a NDD insn which may be optimized to a REX2 insn before the
> +// mov. It may result that a good UB checker object the behavior
> +// "template->start - 1" at the end of match_template.
> +
>  // Move instructions.

While I mentioned adding a comment here as a minimal solution, did you try
to think of better approaches, or some enforcement of this restriction
(like gas_assert() before the expression in question)? You could even go
as far as simply not trying the optimization when t == i386_optab, with no
need to have a comment here (the comment would then be next to that
part of the condition, thus right where it's really relevant). Then anyone
misplacing a new template in the opcode table would simply observe that
an expected optimization doesn't happen, and they surely would find the
conditional with its comment.

For all of the changes below (which are a little hard to review in email),
aiui they only add C as needed. I once again would prefer if that attribute
could be added right as the templates are introduced, with the description
stating the intention and that the actual use of the attribute will be
added later (i.e. as expressed earlier already for NF).

Jan