[PATCH 1/8] Support APX GPR32 with rex2 prefix
Cui, Lili
lili.cui@intel.com
Tue Nov 7 15:31:41 GMT 2023
> 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.
More information about the Binutils
mailing list