[PATCH v4 1/9] Support APX GPR32 with rex2 prefix
Cui, Lili
lili.cui@intel.com
Mon Dec 25 06:14:28 GMT 2023
> On 19.12.2023 13:12, Cui, Lili wrote:
> > @@ -5283,6 +5321,9 @@ md_assemble (char *line)
> > case unsupported_syntax:
> > err_msg = _("unsupported syntax");
> > break;
> > + case unsupported_EGPR_for_addressing:
> > + err_msg = _("extended GPR cannot be used as base/index");
> > + break;
>
> While this one's now suitable for the as_bad() below the switch, ...
>
> > @@ -5336,6 +5377,9 @@ md_assemble (char *line)
> > case invalid_dest_and_src_register_set:
> > err_msg = _("destination and source registers must be distinct");
> > break;
> > + case invalid_pseudo_prefix:
> > + err_msg = _("rex2 pseudo prefix cannot be used here");
> > + break;
>
> ... this one still doesn't really fit the "... for `<insn>'" there. At least the "here"
> needs dropping.
>
Changed to :
"rex2 pseudo prefix cannot be used"
> > @@ -5630,11 +5681,12 @@ 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;
> > + if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
> > + i.rex |= REX_OPCODE;
> > for (x = 0; x < 2; x++)
> > {
> > /* Look for 8 bit operand that uses old registers. */ @@ -5645,7
> > +5697,7 @@ md_assemble (char *line)
> > /* 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."),
> > + "instruction requiring REX/REX2 prefix."),
> > register_prefix, i.op[x].regs->reg_name);
> >
> > /* Otherwise it is equivalent to the extended register.
> > @@ -5657,11 +5709,11 @@ md_assemble (char *line)
> > }
> > }
> >
> > - if (i.rex == 0 && i.rex_encoding)
> > + if (i.rex == 0 && i.rex2 == 0 && (i.rex_encoding ||
> > + i.rex2_encoding))
> > {
> > /* Check if we can add a REX_OPCODE byte. Look for 8 bit operand
> > that uses legacy register. If it is "hi" register, don't add
> > - the REX_OPCODE byte. */
> > + rex and rex2 prefix. */
> > int x;
> > for (x = 0; x < 2; x++)
> > if (i.types[x].bitfield.class == Reg @@ -5671,6 +5723,7 @@
> > md_assemble (char *line)
> > {
> > gas_assert (!(i.op[x].regs->reg_flags & RegRex));
> > i.rex_encoding = false;
> > + i.rex2_encoding = false;
> > break;
> > }
> >
> > @@ -5678,7 +5731,13 @@ md_assemble (char *line)
> > i.rex = REX_OPCODE;
> > }
> >
> > - if (i.rex != 0)
> > + if (is_apx_rex2_encoding ())
> > + {
> > + build_rex2_prefix ();
> > + /* The individual REX.RXBW bits got consumed. */
> > + i.rex &= REX_OPCODE;
> > + }
> > + else if (i.rex != 0)
> > add_prefix (REX_OPCODE | i.rex);
> >
> > insert_lfence_before (last_insn);
>
> All of this will need re-basing over "x86: properly respect rex/{rex}", with the
> result (I hope) that .insn will then also be covered REX2-wise.
>
Done.
> > @@ -5752,6 +5811,20 @@ parse_insn (const char *line, char *mnemonic,
> bool prefix_only)
> > goto too_long;
> > *mnem_p = '\0';
> >
> > + /* Point l at the closing brace if there's no other separator. */
> > + if (*l != END_OF_INSN && !is_space_char (*l)
> > + && *l != PREFIX_SEPARATOR)
> > + --l;
> > + }
> > + /* Skip the immediate 0x** of {rex2 0x00} prefix. */
> > + else if (*mnemonic == '{'&& is_space_char (*l))
>
> Nit: Missing blank.
>
> > + {
> > + while ( *l != '}')
>
> Nit: Stray blank.
>
> > + ++l;
>
> What if there's no '}' on the line?
>
> > + *mnem_p++ = *l++;
> > + if (mnem_p >= mnemonic + MAX_MNEM_SIZE)
> > + goto too_long;
> > + *mnem_p = '\0';
>
> You skip everything, not just 0xNN. You also skip stuff after other pseudo
> prefixes, if I'm not mistaken. That's both too lax. However, skipping isn't an
> option here anyway. Either we actually respect what the user has written, or
> we error out. Skipping therefore is an option only if the provided expression
> (not just plain number!) evaluates to 0. I don't understand anyway why this
> code was added: When I asked about the specific plans, H.J. clearly said the
> form with a constant would be a disassembler- only thing for now.
>
It should be only supported in disassembler, I will drop it in assembler.
> > @@ -7005,6 +7082,43 @@ 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)
>
> Hmm, I thought I had asked before to make functions with boolean return
> values have a return type of bool, and then use "true" for success. An
> alternative would be to return the error indicator, rather than putting it in
> i.error here.
>
> Then again I realize this is in line with VEX_check_encoding() and
> check_VecOperands() (which I think would better be changed, but anyway).
>
Changed it to bool. For the rest, it's a bit strange to only change check_EgprOperands. Can this place be left unchanged? Or should I submit a new patch and change the old one first?
> > @@ -7149,6 +7263,14 @@ match_template (char mnem_suffix)
> > continue;
> > }
> >
> > + /* Check if pseudo prefix {rex2} is valid. */
> > + if (t->opcode_modifier.noegpr && i.rex2_encoding)
> > + {
> > + i.error = invalid_pseudo_prefix;
>
> What is this needed for? I.e. why can't you pass the value ...
>
> > + specific_error = progress (i.error);
>
> ... in here directly (as is done elsewhere as well)?
>
Done.
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-rex2.s
> > @@ -0,0 +1,86 @@
> > +## REX2.B3 bit
> > + sub (%r10), %r31
> > + sub (%r13), %r31
>
> Nit: There's still an indentation anomaly here.
>
Done.
> > --- a/gas/testsuite/gas/i386/x86-64-inval-pseudo.s
> > +++ b/gas/testsuite/gas/i386/x86-64-inval-pseudo.s
> > @@ -1,4 +1,8 @@
> > .text
> > {disp16} movb (%ebp),%al
> > {disp16} movb (%rbp),%al
> > +
> > + /* Instruction not support APX. */
> > + {rex2} xsave (%r15, %rbx)
> > + {rex2} xsave64 (%r15, %rbx)
> > .p2align 4,0
>
> Aren't these dealt with (in a more complete fashion) in x86-64-pseudos-
> bad.s?
>
Removed.
> > --- a/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > +++ b/gas/testsuite/gas/i386/x86-64-pseudos-bad.s
> > @@ -5,3 +5,77 @@ pseudos:
> > {rex} vmovaps %xmm7,%xmm2
> > {rex} vmovaps %xmm17,%xmm2
> > {rex} rorx $7,%eax,%ebx
> > + {rex2} vmovaps %xmm7,%xmm2
> > + {rex2} xsave (%rax)
> > + {rex2} xsaves (%ecx)
> > + {rex2} xsaves64 (%ecx)
> > + {rex2} xsavec (%ecx)
> > + {rex2} xrstors (%ecx)
> > + {rex2} xrstors64 (%ecx)
>
> Here.
>
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -144,6 +144,12 @@ struct instr_info
> > /* Bits of REX we've already used. */
> > uint8_t rex_used;
> >
> > + /* Record W R4 X4 B4 bits for rex2. */ unsigned char rex2;
> > + /* Bits of REX2 we've already used. */ unsigned char rex2_used;
>
> When you say REX2, one ought to be permitted to imply you mean the prefix,
> not the struct field. That's ambiguous here, though - bit positions used match
> those in rex2, not those in the REX2 payload.
> IOW either you use lower case to make more obvious that you mean the other
> struct field, or you say e.g. "REX2 prefix bits we've already used." Albeit that
> would still be imprecise, as other REX2 prefix bits' use is recorded in rex_used.
>
Changed to
/* Bits of rex2 we've already used. */
unsigned char rex2_used;
> > @@ -265,8 +272,13 @@ struct dis_private {
> > { \
> > if (value) \
> > { \
> > - if ((ins->rex & value)) \
> > + if (ins->rex & value) \
> > ins->rex_used |= (value) | REX_OPCODE; \
>
> Like is done here, ...
>
> > + if (ins->rex2 & value) \
> > + { \
> > + ins->rex2_used |= value; \
>
> other uses of "value" also want parenthesizing, unless not used with any kind
> of operator (e.g. in the if() above).
>
Done.
> > @@ -276,6 +288,9 @@ struct dis_private { #define EVEX_b_used 1
> > #define EVEX_len_used 2
> >
> > +/* M0 in rex2 prefix represents map0 or map1. */ #define REX2_M 0x8
>
> Extending an earlier comment: This really should go next to REX_W and
> friends. In principle the assembler could want to use this constant as well,
> hence why it would better go the opcode/i386.h anyway.
>
Done.
> > @@ -4196,19 +4221,19 @@ static const struct dis386 x86_64_table[][2] =
> > {
> >
> > /* X86_64_E8 */
> > {
> > - { "callP", { Jv, BND }, 0 },
> > - { "call@", { Jv, BND }, 0 }
> > + { "callP", { Jv, BND }, PREFIX_REX2_ILLEGAL },
>
> This, ...
>
> > + { "call@", { Jv, BND }, PREFIX_REX2_ILLEGAL }
> > },
> >
> > /* X86_64_E9 */
> > {
> > - { "jmpP", { Jv, BND }, 0 },
> > - { "jmp@", { Jv, BND }, 0 }
> > + { "jmpP", { Jv, BND }, PREFIX_REX2_ILLEGAL },
>
> ... this, and ...
>
> > + { "jmp@", { Jv, BND }, PREFIX_REX2_ILLEGAL }
> > },
> >
> > /* X86_64_EA */
> > {
> > - { "{l|}jmp{P|}", { Ap }, 0 },
> > + { "{l|}jmp{P|}", { Ap }, PREFIX_REX2_ILLEGAL },
> > },
>
> ... this change isn't really needed, is it? The marker is only needed on 64-bit
> insns (i.e. respective slots 2 of x86_64_table[] entries).
>
Done.
Thanks,
Lili.
More information about the Binutils
mailing list