RE: [PATCH] Support APX PUSHP/POPP

["Cui, Lili" <lili.cui@intel.com>]

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


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

Thanks,
Lili.