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

[Jan Beulich <jbeulich@suse.com>]

On 10.11.2023 06:43, Hu, Lin1 wrote:
>> On 02.11.2023 12:29, Cui, Lili wrote:

Btw, your shrinking of reply context above from here is problematic. Someone
reading just this mail can't tell who ...

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

... originally said this.

> Looking at other current NDD instructions, it should be possible to use evex encoding even if it doesn't have rex2 encoding.

Should be possible - yes. But why would you do such a transformation? That's
not an optimization at all, afaict. And we shouldn't alter what the
programmer wrote if the result isn't in at least some respect deemed better
than the original. Considering this, the helper function may want further
naming differently than already suggested, to e.g. convert_NDD_to_REX2().

>>> +      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.
>>
> 
> Indeed, adcx's legacy insn can't support rex2.
> 
> For my problem, I prefer to re-order templates order, because, I hadn't thought of a way to simply move t to the farthest same base_opcode template for the moment. The following is a tentative scenario: the order will be ndd evex - rex2 - evex.

Yes, this matches my understanding / expectation.

> And I will need a tmp_variable to avoid the insn doesn't match the rex2, let me backtrack the match's result and the value of i.

This, however, I'm not convinced of. I'd rather see this vaguely in line
with 58bceb182740 ("x86: prefer VEX encodings over EVEX ones when
possible"): Do another full matching round with the removed operand,
arranging for "internal error" to be raised in case that fails. Your
approach would, I think, result in silent bad code generation in case
something went wrong. Thing is - you don't even need to advance (or
backtrack) t in that case

>>> +	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.)
>>
> 
> Indeed, it's safer, I've exchanged the order of execution, do you have any other comments on the assertions (If I understand correctly, there is a desire for some gcc_assert?), for the time being I can guarantee that the two indexes are definitely in range, is there anything else that needs to be judged?

That was a remark towards possible future changes, independent of your
work here. I merely want to make sure that possibly introducing such an
assertion wouldn't require code changes elsewhere when that can be
easily avoided right away.

>>> @@ -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).
>>
> 
> It's a mistake, I have fixed it. The conditions will be. I will try later, after the NF patch is done, to see if the constraint i.has_nf can be removed or not.
> 
>        /* 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))
> +      if (!i.no_optimize && i.vec_encoding != vex_encoding_evex
> +         && optimize && optimize_NDD_to_nonNDD (t))
>         {

Regardless of what the final expression is going to be, please keep the
check of "optimize" first, such that the common case of optimization
being disabled will be impacted as little as possible.

>>> +	{
>>> +	  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).
>>
> 
> Yes, if we reorder the template's order, I will remove the line. Only one example of a possible implementation is given here:
> 
>         }
> 
> +      bool have_converted_NDD_to_nonNDD = false;
> +      i386_insn tmp_i;
> +
> +      if (!i.no_optimize && i.vec_encoding != vex_encoding_evex
> +         && optimize && !have_converted_NDD_to_nonNDD
> +         && convert_NDD_to_nonNDD (t))
> +       {
> +         have_converted_NDD_to_nonNDD = true;
> +         tmp_i = i;
> +       }
> +
>        /* We've found a match; break out of loop.  */
>        break;
>      }
> @@ -7787,6 +7802,9 @@ match_template (char mnem_suffix)
>        return NULL;
>      }
> 
> +  if (have_converted_to_nonNDD)
> +    i = tmp_i;
> +
>    if (!quiet_warnings)

I have to admit that I don't understand what the goal is of this playing
with i and tmp_i.

>> 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).
> 
> After the changes are finalized, I'll break out this part of the modification that adds the C to lili so she can put it where it belongs.

Hmm, that will need doing early, as the NDD patch is hopefully going to
land soon-ish. Same for the template re-ordering (which will need
explaining when pulled ahead, but it will want pulling ahead to reduce
churn).

Jan