[PATCH] Support APX PUSHP/POPP

[Jan Beulich]

On 29.11.2023 04:08, Cui, Lili wrote:
>> On 28.11.2023 14:14, Cui, Lili wrote:
>>>>
>>>>>>>>> @@ -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 ...
>>>
>>> How about REX2_ IGNORED ?
>>
>> Perhaps. It's no better or worse than REX2_SPECIAL. Just make sure you add a
>> comment explaining what it's to be used for.
>>
> 
> Done.
> 
>>>> 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).
>>>>
>>>
>>> The current implementation is that no matter whether rex.w is consumed or
>> not, {rex2} will be printed as long as there is no Egpr. Of course, this place may
>> be changed as discussed later.
>>
>> That's a bug then. If there is a REX2 prefix with just REX2.W set, and if that bit
>> is properly consumed, no {rex2} should be printed imo. That's despite the
>> same thing being expressable (in the common case; not here) with REX.W.
>> Anything beyond that depends on the wider question of how to deal with
>> _unused_ REX2 payload bits.
>>
> 
> For the last two instructions, although rex.w is consumed, we still need to print {rex2} to distinguish them.
> 
> 0000000000000000 <_start>:
>    0:   48 50                                 rex.W push %rax
>    2:   d5 08 50                           pushp  %rax
>    5:   48 6a 01                           rex.W push $0x1
>    8:   d5 08 6a 01                     {rex2} push $0x1
>    c:   48 c7 00 12 00 00 00                 movq   $0x12,(%rax)
>   13:   d5 08 c7 00 12 00 00 00         {rex2} movq $0x12,(%rax)

Well, no, I don't really agree (especially not for the last one). But
that goes back to the more general question on the printing of {rex2}
without also indicating unconsumed bits.

>>>>>>>> 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).
>>>>
>>>
>>> Haha, StaticRounding is really special, I tried "#define Rex2Req Masking" and
>> found that it will be used in i386-gen.c to identify EVEX (Broadcast...), then I
>> tried VexW and SIB found that they are all used without precheck whether it
>> was an vex instruction. Finally I wanted to re-use StaticRounding and found
>> out that hulin already uses it for legacy insns.
>>
>> Hmm, I'm sorry for the trouble. I'm inclined to say OperandConstraint with a
>> new #define it is then. Once everything's in I could then still see whether I can
>> (reasonably) make e.g. Masking work here.
>>
> 
> Then I will create a new attribute Rex2 for it.

???

Jan