[PATCH 1/8] Support APX GPR32 with rex2 prefix
Jan Beulich
jbeulich@suse.com
Fri Nov 3 14:19:36 GMT 2023
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?
> @@ -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?
> @@ -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.)
Speaking of v1: Can you please make sure you have correct version tags
on submissions of updated patch versions?
> @@ -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.
> @@ -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?
> @@ -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?
> @@ -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.
> @@ -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.
> @@ -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.
> @@ -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?
> + 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.
> + {
> + 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.
> @@ -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).
> --- 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?
> --- 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.
> --- 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.
> @@ -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.)
Jan
More information about the Binutils
mailing list