[PATCH 7/8] Support APX NDD optimized encoding.
Hu, Lin1
lin1.hu@intel.com
Fri Nov 10 05:43:59 GMT 2023
> 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.)
>
Of course we can put the optimization patch after the NF patch. For now, we are just putting it here for review.
>
> 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.)
>
Looking at other current NDD instructions, it should be possible to use evex encoding even if it doesn't have rex2 encoding.
>
> > + && i.reg_operands >= 2
> > + && i.types[i.operands - 1].bitfield.class == Reg)
>
> Isn't this implicit from the VexVVVV check further up?
>
Yes.
>
> > + {
> > + 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;
>
> ?
>
OK.
>
> > + 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. 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.
>
> > + /* 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?
>
I forgot to modify it.
>
> > + 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?
> > @@ -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))
{
>
> > + {
> > + 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)
>
> > + 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.)
>
Have modified.
>
> > --- 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.
>
If we don't reorder i386.tbl, I will consider to modify t = current_templates->start - 1 to
If (t != current_templates->start)
t = current_templates->start - 1;
>
> 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.
BRs,
Lin
More information about the Binutils
mailing list