[PATCH 7/8] Support APX NDD optimized encoding.
Hu, Lin1
lin1.hu@intel.com
Wed Nov 15 02:52:29 GMT 2023
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 14, 2023 6:51 PM
> To: Hu, Lin1 <lin1.hu@intel.com>; Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> binutils@sourceware.org
> Subject: Re: [PATCH 7/8] Support APX NDD optimized encoding.
>
> On 14.11.2023 03:28, Hu, Lin1 wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >>
> >> On 10.11.2023 06:43, Hu, Lin1 wrote:
> >>>> On 02.11.2023 12:29, Cui, Lili wrote:
> >>>>> + 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;
>
> DYM decrement rather than increment for these? We're trying to go from
> 3 to 2 operands, after all.
>
Here's a backtrace to considering for possible other opcode_space (0f38,...) instructions that can't accept the r16+ register, but can accept other rex registers or the normal. I decrement i.operands and i.reg_operands in convert_NDD_to_REX2. If the legacy or rex version of the insn can't support rex2 registers, I won't optimize it. So I need to increment these.
>
> > + ++i.tm.operands;
>
> Why is this? Aren't we ahead of filling i.tm here?
>
Indeed. I removed it.
>
> > +
> > + 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.
>
> Good. Looking forward to seeing the full change.
>
For some insns like adcx and adox, I'd like to add some details. check_EgprOperands only used to exclude some situation that these insns with gpr32 registers. If we think about optimization in terms of encoding length. Is it safe to assume that some insn with prefixes 66, f2, f3 and their opcode_space isn't legacy-map0 or legacy-map1 won't reduce the length of the code even if they are optimized? If yes, I think the code can be simplified like:
/* 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
&& convert_NDD_to_REX2 (t))
{
specific_error = progress (internal_error);
continue;
}
For those instructions that don't need to be optimized, like adcx and adox we just don't swap the order, so we don't need check_EgprOperands and backtrack, and convert_NDD_to_REX2 has the same return value as before.
PS. So shouldn't the name of the function be convert_NDD_to_legacy.
>
> > 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?
>
> See my comments to Lili on already the original code (which you further
> modify) here. There cannot be a dependency on current_templates here, imo.
> Lili - the fact Lin needs the modification above actually looks to support my view
> on this.
This part of the code won't be in the NDD optimize patch but rather should be in the NDD patch. So I'd like to skip that part of the discussion for now until I get a new base branch.
While I still have questions about the previous discussion, I will then send out the current patch.
BRs,
Lin
More information about the Binutils
mailing list