[PATCH] Support APX PUSHP/POPP
Jan Beulich
jbeulich@suse.com
Tue Nov 28 08:34:41 GMT 2023
On 28.11.2023 03:32, Cui, Lili wrote:
>>>>> --- a/opcodes/i386-dis.c
>>>>> +++ b/opcodes/i386-dis.c
>>>>> @@ -1931,23 +1931,23 @@ static const struct dis386 dis386[] = {
>>>>> { "dec{S|}", { RMeSI }, 0 },
>>>>> { "dec{S|}", { RMeDI }, 0 },
>>>>> /* 50 */
>>>>> - { "push{!P|}", { RMrAX }, 0 },
>>>>> - { "push{!P|}", { RMrCX }, 0 },
>>>>> - { "push{!P|}", { RMrDX }, 0 },
>>>>> - { "push{!P|}", { RMrBX }, 0 },
>>>>> - { "push{!P|}", { RMrSP }, 0 },
>>>>> - { "push{!P|}", { RMrBP }, 0 },
>>>>> - { "push{!P|}", { RMrSI }, 0 },
>>>>> - { "push{!P|}", { RMrDI }, 0 },
>>>>> + { "push!P", { RMrAX }, 0 },
>>>>> + { "push!P", { RMrCX }, 0 },
>>>>> + { "push!P", { RMrDX }, 0 },
>>>>> + { "push!P", { RMrBX }, 0 },
>>>>> + { "push!P", { RMrSP }, 0 },
>>>>> + { "push!P", { RMrBP }, 0 },
>>>>> + { "push!P", { RMrSI }, 0 },
>>>>> + { "push!P", { RMrDI }, 0 },
>>>>> /* 58 */
>>>>> - { "pop{!P|}", { RMrAX }, 0 },
>>>>> - { "pop{!P|}", { RMrCX }, 0 },
>>>>> - { "pop{!P|}", { RMrDX }, 0 },
>>>>> - { "pop{!P|}", { RMrBX }, 0 },
>>>>> - { "pop{!P|}", { RMrSP }, 0 },
>>>>> - { "pop{!P|}", { RMrBP }, 0 },
>>>>> - { "pop{!P|}", { RMrSI }, 0 },
>>>>> - { "pop{!P|}", { RMrDI }, 0 },
>>>>> + { "pop!P", { RMrAX }, 0 },
>>>>> + { "pop!P", { RMrCX }, 0 },
>>>>> + { "pop!P", { RMrDX }, 0 },
>>>>> + { "pop!P", { RMrBX }, 0 },
>>>>> + { "pop!P", { RMrSP }, 0 },
>>>>> + { "pop!P", { RMrBP }, 0 },
>>>>> + { "pop!P", { RMrSI }, 0 },
>>>>> + { "pop!P", { RMrDI }, 0 },
>>>>> /* 60 */
>>>>> { X86_64_TABLE (X86_64_60) },
>>>>> { X86_64_TABLE (X86_64_61) },
>>>>> @@ -10621,6 +10621,19 @@ putop (instr_info *ins, const char
>>>> *in_template, int sizeflag)
>>>>> case 'P':
>>>>> if (l == 0)
>>>>> {
>>>>> + /* For pushp and popp, do not print {rex2} for them. */
>>>>> + if (ins->address_mode == mode_64bit && !cond
>>>>
>>>> I don't think the mode_64bit check is needed here, as without that
>>>> REX.W cannot possibly be set (nor can a REX2 prefix be present).
>>>
>>> Done.
>>>
>>>>
>>>>> + && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
>>>>> + {
>>>>> + *ins->obufp++ = 'p';
>>>>> + ins->rex2 |= 16;
>>>>
>>>> Please no new use of magic constants. Have a #define with a suitable
>>>> name, and use that here. Also I think the comment you have ahead of
>>>> the if() actually belongs here?
>>>>
>>>
>>> How about " #define IMPLICIT_REX2 16", PUSHP/POPP can share it with
>> JMPABS.
>>
>> But what's implicit about the REX2 prefix here?
>
> PUSHP/POPP requires rex2.w ==1 and we don't want to print {rex2} for it. JMPABS has the same need, so I want to define a common macro for them, or NO_NEED_PRINT_REX2?
Yes, we want a shared constant here (as much as we want a shared way of
dealing with the need to emit REX2 in the assembler). Still, the naming
wants to fit not only the goal (to suppress the printing of {rex2}), but
also where the bit is actually stored (in ->rex2). Therefore its name
wants to fit with the other names used for bits in that field. Which
(to me) first of all means it wants to start with REX_ (or REX2_).
REX_SPECIAL or REX2_SPECIAL might be an option, but might also be too
generic (i.e. becoming an issue down the road). But I think this should
give you an idea ...
Then again, why is this constant needed for PUSHP/POPP, which have
REX2.W set anyway, and hence that bit alone (when properly marked as
consumed) should already allow to omit {rex2}. The question of adding
a new constant (and how to suitably name it) would then be fully
constrained to the JMPABS patch (for now, i.e. until such time that a
2nd insn appears which requires an otherwise empty REX2 prefix).
>>>>> --- a/opcodes/i386-opc.tbl
>>>>> +++ b/opcodes/i386-opc.tbl
>>>>> @@ -225,6 +225,7 @@ push, 0x68, i186&No64,
>>>>> DefaultSize|No_bSuf|No_sSuf|No_qSuf, { Imm16|Imm32 } push, 0x6,
>>>> No64,
>>>>> DefaultSize|No_bSuf|No_sSuf|No_qSuf, { SReg } // In 64bit mode, the
>>>> operand size is implicitly 64bit.
>>>>> push, 0x50, x64, No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64 }
>>>>> +pushp, 0x50, APX_F, No_bSuf|No_lSuf|No_sSuf|Rex2, {Reg64 }
>>>>
>>>> Since Reg16 isn't allowed, you also want No_wSuf here (and below).
>>>> Note also the missing blank after the opening figure brace.
>>>>
>>>
>>> Done.
>>>
>>>> The new Rex2 attribute is not only wasteful (it can easily be a new
>>>> enumerator used with OperandConstraint), but also misleading. We
>>>> don't just need REX2 here, but we need it with REX2.W set. Even if
>>>> from the tc-i386.c changes it looks as if that was happening
>>>> implicitly (presumably due to the absence of NoRex64), naming still needs
>> to properly reflect the purpose.
>>>>
>>>
>>> You are right, rex2.w is set in process_suffix, since there is no NoRex64.
>>>
>>> How about Rex2W?
>>> if (i.tm.opcode_modifier.rex2w)
>>> {
>>> i.rex2_encoding = true;
>>> i.rex |= REX_W; // add NoRex64 back, and set REX_W here.
>>> }
>>>
>>>
>>> Or just add a special judgment?
>>>
>>> if (t->mnem_off == MN_pushp || t->mnem_off == MN_popp) {
>>> i.rex2_encoding = true;
>>> i.rex |= REX_W; // add NoRex64 back, and set REX_W here.
>>> }
>>
>> I'd prefer the latter over a new attribute (albeit once again with the comment
>> actually matching code), and perhaps I view the latter equal to my earlier
>> suggestion.
>>
>
> Ok, put them in process_operands, there's already a special handler here to change the prefix.
>
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index 0dde2a9ad44..2f2b1b04d10 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -8715,6 +8715,13 @@ process_operands (void)
> i.tm.operands++;
> }
>
> + /* PUSHP/POPP requires rex2.w == 1. */
> + if (i.tm.mnem_off == MN_pushp || i.tm.mnem_off == MN_popp)
> + {
> + i.rex2_encoding = true;
> + i.rex |= REX_W;
> + }
Well, I'm sorry for not considering JMPABS earlier on, but with that
also needing dealing with, I think I view my alternative suggestion as
preferable. That'll scale better when also considering that down the
road further such insns may appear. Whether it's actually
OperandConstraint that we leverage here is secondary (it's not ideal
because there's nothing operand related here). I'd be perfectly okay
with some other attribute being suitably overloaded, whereas I
continue to think that introducing new attributes should preferably
be limited to either cases where more than just two or three
templates use them or cases where otherwise it's impossible to avoid
ambiguities. From earlier changes of mine the underlying reason
ought to be pretty clear: Each new attribute consumes storage, and
with thousands of templates growth of storage requirements should
be balanced with how frequently an attribute is actually going to
have a non-zero value. For example, with is_evex_encoding() gone a
brief inspection suggests that it might be possible to overload
Masking (or maybe Broadcast): They're applicable to EVEX templates
only, and the class of insns we're discussing here is never going to
be EVEX (or VEX). IOW not much different from the overloading of
StaticRounding. Such an overload may then well be named Rex2 (as you
had it, and considering its intended use also for JMPABS, plus
taking into consideration that REX2.W will be set simply because of
the absence of NoRex64).
The other issue I have with your approach is that you again (ab)use
i.rex2_encoding: As expressed before, that's representing {rex2}, i.e.
a weak request (aka hint). Whereas the two insns here _require_ a REX2
prefix. Doing so may end up being tolerable, but would require extra
justification and/or commenting.
Finally putting the logic in process_operands() is also misleading:
There's no processing of operands here. Pre-existing abuse of the
function (as you appear to indicate exists, albeit I didn't go check)
is not really a good excuse, I'm afraid.
Jan
More information about the Binutils
mailing list