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

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

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, November 10, 2023 5:54 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> binutils@sourceware.org; Cui, Lili <lili.cui@intel.com>
> Subject: Re: [PATCH 7/8] Support APX NDD optimized encoding.
> 
> 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.
>

Thanks for your advises.

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

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

I tried to reorder the templates and modify the code as follows:

@ -7728,6 +7765,40 @@ 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,
+        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)
+       {
+         unsigned int readonly_var = convert_NDD_to_REX2 (t);
+         if (readonly_var != ~0)
+           {
+             if (!check_EgprOperands (t + 1))
+               {
+                 specific_error = progress (internal_error);
+                 continue;
+               }
+             ++i.operands;
+             ++i.reg_operands;
+             ++i.tm.operands;
+
+             if (readonly_var == 1)
+               swap_2_operands (0, 1);
+           }
+       }

convert_NDD_to_REX2 return readonly_var now. check_EgprOperands aims to exclude some insns like adcx and adox. Because their opcode_space is legacy-map2 can't support rex2.

And I need some modifications in tc-i386.c after reorder i386-opc.tbl.

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 7a86aff1828..d98950c7dfd 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -14401,7 +14401,9 @@ static bool check_register (const reg_entry *r)

   if (r->reg_flags & RegRex2)
     {
-      if (is_evex_encoding (current_templates->start))
+      if (is_evex_encoding (current_templates->start)
+         && ((current_templates->start + 1 >= current_templates->end)
+             || (is_evex_encoding (current_templates->start + 1))))
        i.vec_encoding = vex_encoding_evex;

       if (!cpu_arch_flags.bitfield.cpuapx_f

What's your opinion?

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

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

This variable has been removed in the new version, so there's no need to think about it.

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

OK. These changes, if there are no major problems, I'll give a patch (This patch will be attached to this email for double-check.) to lili, and write in the changelog the reason why we made these changes.

BRs,
Lin