[PATCH 1/8] Support APX GPR32 with rex2 prefix
Cui, Lili
lili.cui@intel.com
Mon Nov 6 15:20:39 GMT 2023
> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
>
> On 02.11.2023 12:29, Cui, Lili wrote:
> > @@ -406,6 +409,11 @@ struct _i386_insn
> > /* Compressed disp8*N attribute. */
> > unsigned int memshift;
> >
> > + /* No CSPAZO flags update.*/
> > + bool has_nf;
> > +
> > + bool has_zero_upper;
> > +
>
> Can both please be introduced when they're needed, not randomly ahead of
> time?
Moved has_nf to patch 2/8 and deleted has_zero_upper.
> > @@ -2375,6 +2388,9 @@ register_number (const reg_entry *r)
> > if (r->reg_flags & RegRex)
> > nr += 8;
> >
> > + if (r->reg_flags & RegRex2)
> > + nr += 16;
> > +
> > if (r->reg_flags & RegVRex)
> > nr += 16;
>
> Perhaps fold to
>
> if (r->reg_flags & (RegVRex | RegRex2))
> nr += 16;
>
> ? Irrespective an assertion may be worthwhile that both flags aren't set at the
> same time?
Done.
>
> > @@ -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);
> Speaking of v1: Can you please make sure you have correct version tags on
> submissions of updated patch versions?
>
I used git to send all the patches at once( git send-email --cover-letter --annotate --to="..." -8), which only has the opportunity to change the version of the cover letter patch. To change the version of each patch, I can send them one by one next time. By the way, do you have a better way? Or how did you modify them? Thanks.
> > @@ -4423,12 +4460,16 @@ optimize_encoding (void)
> > i.suffix = 0;
> > /* Convert to byte registers. */
> > if (i.types[1].bitfield.word)
> > - j = 16;
> > - else if (i.types[1].bitfield.dword)
> > + /* There are 32 8-bit registers. */
>
> Please make sure comments are actually correct. With your additions there
> are 40 8-bit registers; prior to that there were 24. The j += 8 further down deal
> with that difference, and the comment here (if one is to be added) wants to
> tell the full truth.
>
Done.
> > @@ -5278,6 +5319,9 @@ md_assemble (char *line)
> > case register_type_mismatch:
> > err_msg = _("register type mismatch");
> > break;
> > + case register_type_of_address_mismatch:
> > + err_msg = _("register type of address mismatch");
> > + break;
>
> I have a concern with wording / naming here: If I saw this in an error message,
> I wouldn't know what is meant. Maybe something along the lines of "cannot
> use an extended GPR for addressing"? And then the enumerator suitabley
> renamed as well?
>
Changed to
+ case unsupported_EGPR_for_addressing:
+ err_msg = _("unsupported EGPR for addressing");
+ break;
> > @@ -5578,7 +5625,7 @@ md_assemble (char *line)
> > as_warn (_("translating to `%sp'"), insn_name (&i.tm));
> > }
> >
> > - if (is_any_vex_encoding (&i.tm))
> > + if (is_any_vex_encoding (&i.tm))
> > {
>
> Stray change, breaking indentation?
>
Done.
> > @@ -5594,6 +5641,13 @@ md_assemble (char *line)
> > return;
> > }
> >
> > + /* Check for explicit REX2 prefix. */
> > + if (i.rex2 || i.rex2_encoding)
>
> This open-codes is_any_apx_rex2_encoding(). But read on.
>
> > + {
> > + as_bad (_("REX2 prefix invalid with `%s'"), insn_name (&i.tm));
>
> There's no REX2 prefix; {rex2} only sets i.rex2_encoding. Question is what case
> the i.rex2 check above is intended to cover. Error message comment, and
> condition want to reflect that.
>
Removed i.rex2 and keep i.rex2_encoding here. Added one invalid testcase for it.
{rex} vmovaps %xmm7,%xmm2
{rex} vmovaps %xmm17,%xmm2
{rex} rorx $7,%eax,%ebx
+ {rex2} vmovaps %xmm7,%xmm2
> > @@ -5633,11 +5687,11 @@ md_assemble (char *line)
> > && (i.op[1].regs->reg_flags & RegRex64) != 0)
> > || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> > || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > - && i.rex != 0))
> > + && (i.rex != 0 || i.rex2 != 0)))
> > {
> > int x;
> > -
> > - i.rex |= REX_OPCODE;
>
> Please don't remove blank lines like this.
>
Done.
> > @@ -5647,9 +5701,11 @@ md_assemble (char *line)
> > gas_assert (!(i.op[x].regs->reg_flags & RegRex));
> > /* In case it is "hi" register, give up. */
> > if (i.op[x].regs->reg_num > 3)
> > - as_bad (_("can't encode register '%s%s' in an "
> > - "instruction requiring REX prefix."),
> > - register_prefix, i.op[x].regs->reg_name);
> > + {
> > + as_bad (_("can't encode register '%s%s' in an "
> > + "instruction requiring REX/REX2 prefix."),
> > + register_prefix, i.op[x].regs->reg_name);
> > + }
>
> There's no need to introduce braces here. Without doing so this will also be
> less of a change.
>
Done.
> > @@ -6989,6 +7056,44 @@ VEX_check_encoding (const insn_template *t)
> > return 0;
> > }
> >
> > +/* Check if Egprs operands are valid for the instruction. */
> > +
> > +static int
> > +check_EgprOperands (const insn_template *t) {
> > + if (t->opcode_modifier.noegpr)
> > + {
>
> This scope effectively covers the entire function. Did you consider
>
> if (!t->opcode_modifier.noegpr)
> return 0;
>
> to aid readability?
>
Done.
> > + for (unsigned int op = 0; op < i.operands; op++)
> > + {
> > + if (i.types[op].bitfield.class != Reg
> > + /* Special case for (%dx) while doing input/output op */
> > + || i.input_output_operand)
>
> Why is this needed? The register table entry for %dx ...
>
> > + continue;
> > +
> > + if (i.op[op].regs->reg_flags & RegRex2)
>
> ... doesn't have this bit set anyway.
>
For this special case i.op is empty, we need continue, otherwise r i.op[op].regs->reg_flags will cause segment fault.
> > + {
> > + 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
> > @@ -14131,6 +14258,13 @@ static bool check_register (const reg_entry *r)
> > i.vec_encoding = vex_encoding_error;
> > }
> >
> > + if (r->reg_flags & RegRex2)
> > + {
> > + if (!cpu_arch_flags.bitfield.cpuapx_f
> > + || flag_code != CODE_64BIT)
> > + return false;
> > + }
>
> Please fold the two if()s into one (unless of course you know that the outer
> one is going to be extended in a subsequent patch).
>
Yes, other code will be added in the outer if with patch2/8.
> > --- a/gas/doc/c-i386.texi
> > +++ b/gas/doc/c-i386.texi
> > @@ -216,6 +216,7 @@ accept various extension mnemonics. For example,
> > @code{avx10.1/512}, @code{avx10.1/256}, @code{avx10.1/128},
> > +@code{apx},
> > @code{amx_int8},
> > @code{amx_bf16},
> > @code{amx_fp16},
> > @@ -1662,7 +1663,7 @@ supported on the CPU specified. The choices for
> @var{cpu_type} are:
> > @item @samp{.lwp} @tab @samp{.fma4} @tab @samp{.xop} @tab
> > @samp{.cx16} @item @samp{.padlock} @tab @samp{.clzero} @tab
> > @samp{.mwaitx} @tab @samp{.rdpru} @item @samp{.mcommit} @tab
> > @samp{.sev_es} @tab @samp{.snp} @tab @samp{.invlpgb} -@item
> > @samp{.tlbsync}
> > +@item @samp{.tlbsync} @tab @samp{.apx}
> > @end multitable
>
> DYM apx_f in both cases?
>
> Also don't you need to also mention {rex2} somewhere in this file?
>
Done.
> > --- a/gas/testsuite/gas/i386/ilp32/x86-64-opcode-inval-intel.d
> > +++ b/gas/testsuite/gas/i386/ilp32/x86-64-opcode-inval-intel.d
> > @@ -11,11 +11,11 @@ Disassembly of section .text:
> > [ ]*[a-f0-9]+: 37 \(bad\)
> >
> > 0+1 <aad0>:
> > -[ ]*[a-f0-9]+: d5 \(bad\)
> > +[ ]*[a-f0-9]+: d5 rex2
> > [ ]*[a-f0-9]+: 0a .byte 0xa
> >
> > 0+3 <aad1>:
> > -[ ]*[a-f0-9]+: d5 \(bad\)
> > +[ ]*[a-f0-9]+: d5 rex2
> > [ ]*[a-f0-9]+: 02 .byte 0x2
> >
> > 0+5 <aam0>:
> > --- a/gas/testsuite/gas/i386/ilp32/x86-64-opcode-inval.d
> > +++ b/gas/testsuite/gas/i386/ilp32/x86-64-opcode-inval.d
> > @@ -11,11 +11,11 @@ Disassembly of section .text:
> > [ ]*[a-f0-9]+: 37 \(bad\)
> >
> > 0+1 <aad0>:
> > -[ ]*[a-f0-9]+: d5 \(bad\)
> > +[ ]*[a-f0-9]+: d5 rex2
> > [ ]*[a-f0-9]+: 0a .byte 0xa
> >
> > 0+3 <aad1>:
> > -[ ]*[a-f0-9]+: d5 \(bad\)
> > +[ ]*[a-f0-9]+: d5 rex2
> > [ ]*[a-f0-9]+: 02 .byte 0x2
> >
> > 0+5 <aam0>:
>
> These expectations match the ones of the same test in the parent directory.
> Hence instead of adjusting each in both places, please have the ones here
> reference the parent directory files.
>
They are used to test illegal opcodes for x86-64. Since D5 now makes sense, these two test cases were removed.
# All the followings are illegal opcodes for x86-64.
aad0:
aad
aad1:
aad $2
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
>
> As before I'll look at the disassembler changes separately. This patch is simply
> too big.
>
Ok
> > @@ -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 }
Thanks,
Lili.
More information about the Binutils
mailing list