RE: [PATCH 1/8] Support APX GPR32 with rex2 prefix

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

> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> 
> On 07.11.2023 09:16, Cui, Lili wrote:
> >>>> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> >>>>
> >>>> On 02.11.2023 12:29, Cui, Lili wrote:
> >>>>> @@ -4158,6 +4182,19 @@ build_evex_prefix (void)
> >>>>>      i.vex.bytes[3] |= i.mask.reg->reg_num;  }
> >>>>>
> >>>>> +/* Build (2 bytes) rex2 prefix.
> >>>>> +   | D5h |
> >>>>> +   | m | R4 X4 B4 | W R X B |
> >>>>> +*/
> >>>>> +static void
> >>>>> +build_rex2_prefix (void)
> >>>>> +{
> >>>>> +  i.vex.length = 2;
> >>>>> +  i.vex.bytes[0] = 0xd5;
> >>>>> +  i.vex.bytes[1] = ((i.tm.opcode_space << 7)
> >>>>> +		    | (i.rex2 << 4) | i.rex);
> >>>>> +}
> >>>>
> >>>> I may have asked on v1 already: For emitting REX we don't resort to
> >>>> (ab)using i.vex. Is that really necessary? (If so, a comment next
> >>>> to the field declaration may be warranted.)
> >>>>
> >>> Added comment for it.
> >>>
> >>>   /* For the W R X B bits, the variables of rex prefix will be reused.  */
> >>>   i.vex.bytes[1] = ((i.tm.opcode_space << 7)
> >>>                     | (i.rex2 << 4) | i.rex);
> >>
> >> How does the comment relate to the (ab)use of i.vex?
> >>
> > Ah ha, it's i.vex, not i.rex. At first I thought rex2 should have its own variable,
> but in the output_insn function they have the same special handling of
> i.tm.opcode_space as VEX. Reusing i.vex can reduce some ugly code.
> 
> Things like this are very helpful to explain in the patch description.
> 

Done.

> >>>>> +	    {
> >>>>> +	      i.error = register_type_mismatch;
> >>>>> +	      return 1;
> >>>>> +	    }
> >>>>> +	}
> >>>>> +
> >>>>> +      if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
> >>>>> +	  || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
> >>>>> +	{
> >>>>> +	  i.error = register_type_of_address_mismatch;
> >>>>> +	  return 1;
> >>>>> +	}
> >>>>> +
> >>>>> +      /* Check pseudo prefix {rex2} are valid.  */
> >>>>> +      if (i.rex2_encoding)
> >>>>> +	{
> >>>>> +	  i.error = invalid_pseudo_prefix;
> >>>>> +	  return 1;
> >>>>> +	}
> >>>>
> >>>> Further up in md_assemble() {rex} or {rex2} is simply ignored when
> >>>> wrong to apply. Why would an inapplicable {rex2} be treated as an
> >>>> error here? This would then also ...
> >>>>
> >>>>> @@ -7125,7 +7230,7 @@ match_template (char mnem_suffix)
> >>>>>        /* Do not verify operands when there are none.  */
> >>>>>        if (!t->operands)
> >>>>>  	{
> >>>>> -	  if (VEX_check_encoding (t))
> >>>>> +	  if (VEX_check_encoding (t) || check_EgprOperands (t))
> >>>>>  	    {
> >>>>>  	      specific_error = progress (i.error);
> >>>>>  	      continue;
> >>>>
> >>>> ... eliminate the need for this change, which is kind of bogus anyway:
> >>>> There are no operands here, so calling a function of the given name
> >>>> is at least suspicious.
> >>>>
> >>>
> >>> We have these tests and I'm confused whether to remove them or not.
> >>>
> >>> +       #All opcodes in the row 0xf3* prefixed REX2 are illegal.
> >>> +       {rex2} wrmsr
> >>> +       {rex2} rdtsc
> >>> +       {rex2} rdmsr
> >>> +       {rex2} sysenter
> >>> +       {rex2} sysexitl
> >>> +       {rex2} rdpmc
> >>
> >> They should all stay. But as to my comment: There's no use of any
> >> eGPR here. If you want to abuse that function and if there's no
> >> better descriptive name for it, then once again at least a comment is
> needed.
> >> (Considering this, the attribute's name NoEgpr is probably also
> >> misleading in the cases here, i.e. when there are no operands. Hence,
> >> if not to be renamed, requires yet another comment in i386-opc.h.)
> >>
> > This question also confused me , some instructions only support Acc register,
> but we need to add NoEgpr for them, this seems a bit strange. if we use
> NoRex2 , it doesn't fit the vex and evex instructions either. So I will add
> comments to it for now.
> >
> > +         /* When there are no operands, we still need to use the
> > +            check_EgprOperands function to check whether {rex2} is
> > + valid.  */
> >           if (VEX_check_encoding (t) || check_EgprOperands (t))
> >
> > -  /* egprs (r16-r31) on instruction illegal.  */
> > +  /* egprs (r16-r31) on instruction illegal. We also use it to judge
> > +     whether the instruction supports pseudo-prefix {rex2}.  */
> >    NoEgpr,
> 
> This looks okay commentary-wise, but as per above we first need to settle on
> whether an inapplicable {rex2} shouldn't simply be ignored.
> 
> >>>>> @@ -1008,10 +1012,35 @@ get_element_size (char **opnd, int lineno)
> >>>>>    return elem_size;
> >>>>>  }
> >>>>>
> >>>>> +static bool
> >>>>> +if_entry_needs_special_handle (const unsigned long long opcode,
> >>>>> +unsigned
> >>>> int space,
> >>>>> +			       const char *cpu_flags)
> >>>>> +{
> >>>>> +  /* Prefixing XSAVE* and XRSTOR* instructions with REX2 triggers
> >>>>> +#UD.  */
> >>>>> +  if (strcmp (cpu_flags, "XSAVES") >= 0
> >>>>> +      || strcmp (cpu_flags, "XSAVEC") >= 0
> >>>>> +      || strcmp (cpu_flags, "Xsave") >= 0
> >>>>> +      || strcmp (cpu_flags, "Xsaveopt") >= 0
> >>>>
> >>>> Upon further thought for these (and maybe even ...
> >>>>
> >>>>> +      || !strcmp (cpu_flags, "3dnow")
> >>>>> +      || !strcmp (cpu_flags, "3dnowA"))
> >>>>
> >>>> ... for these, but see also below) it might be better to add the
> >>>> attribute right in the opcode table.
> >>>>
> >>>> As to the 3dnow insns - I think I'd like to revise my earlier
> >>>> suggestion to also tag those. Like e.g. FPU insns they're pretty
> >>>> normal GPR-wise, so allowing them to be used like that would appear
> >>>> only consistent. Otherwise, if we were concerned of AMD extensions
> >>>> in general, SSE4a insns (and maybe further
> >>>> ones) would also need excluding. (Additionally recall that there's
> >>>> an overlap between 3dnowa and SSE, which would result in another
> >>>> [apparent] inconsistency when excluding 3dnow insns here.)
> >>>>
> >>>
> >>> I see, for example  I think I need to split this table into two
> >>> parts, one is for
> >> SSE and one is for 3dnowA, then add noegpr to the SSE one, right?
> >>> pextrw, 0xfc5, SSE|3dnowA,
> >>> Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|NoRex64, { Imm8,
> >> RegMMX,
> >>> Reg32|Reg64 }
> >>
> >> I'm afraid I don't understand the question. All I've asked for is
> >> that the special treatment of 3dnow insns be removed again. Unless
> >> you want to special-case further insns; it's not really clear to me
> >> what's best, as both approaches have noticable downsides (either we
> >> allow to encode something which may never become valid, or we disallow
> something which may become valid).
> >>
> >> In any event adding NoEgpr to any SSE insn sounds wrong to me - aiui
> >> they can all be encoded with REX2.
> >>
> > I need to correct it:  There are some instructions table present both SSE and
> AMD instructions. I need to split them first and then add NoEgpr to AMD
> instructions.
> > Another point is that we have not split the common instructions of AMD and
> Intel, so just adding NoEgpr to 3dnowA and 3dnow does not seem to make
> much sense.
> >
> > Do you want me also to remove this part  and add  NoEgpr in insn table?
> 
> First we need to settle on what to do with 3DNow!, SSE4a, and maybe further
> AMD-only insns (beyond e.g. XOP and TBM ones, which aiui are covered by
> virtue of being VEX[-like], and hence never eligible for eGPR use). Then we can
> sort out how to best express what we have decided to enforce.
> 
> I'm not convinced at all that templates like that for MASKMOVQ would need
> splitting: The difference would be noticeable only if someone disabled SSE, but
> kept 3DNow! and APX_F enabled. We could easily document the resulting
> pitfall instead.
> 

 Do you mean we won't add NoEgpr to the entries like this ? I will try to find a list for AMD-only insns.

pextrw, 0xfc5, SSE|3dnowA, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_sSuf|NoRex64, { Imm8, RegMMX, Reg32|Reg64 }


Lili.