V2 [PATCH] x86: Rename VexOpcode to OpcodePrefix

[H.J. Lu]

On Wed, Oct 14, 2020 at 5:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.10.2020 04:21, H.J. Lu via Binutils wrote:
> > This is the patch I am checking in.
> >
> > gas/
> >
> > * config/tc-i386.c (build_vex_prefix): Replace vexopcode with
> > opcodeprefix.
> > (build_evex_prefix): Likewise.
> > (is_any_vex_encoding): Don't check vexopcode.
> > (output_insn): Handle opcodeprefix.
> >
> > opcodes/
> >
> > * i386-gen.c (opcode_modifiers): Replace VexOpcode with
> > OpcodePrefix.
> > * i386-opc.h (VexOpcode): Renamed to ...
> > (OpcodePrefix): This.
> > (PREFIX_NONE): New.
> > (PREFIX_0X66): Likewise.
> > (PREFIX_0XF2): Likewise.
> > (PREFIX_0XF3): Likewise.
>
> Why 0X66 etc and not just 66?

Free feel to remove 0X.

> > * i386-opc.tbl (Prefix_0X66): New.
> > (Prefix_0XF2): Likewise.
> > (Prefix_0XF3): Likewise.
> > Replace VexOpcode= with OpcodePrefix=.  Use Prefix_0X66 on xorpd.
> > Use Prefix_0XF3 on cvtdq2pd.  Use Prefix_0XF2 on cvtpd2dq.
>
> Is this an arbitrary choice? Such things would be helpful to be
> spelled out in a little bit of a description, not just for reviewing

How about

// Add 0x66/0xf2/0xf3 prefix to non-VEX/EVEX/prefix instructions.
#define Prefix_0X66 OpcodePrefix=PREFIX_0X66
#define Prefix_0XF2 OpcodePrefix=PREFIX_0XF2
#define Prefix_0XF3 OpcodePrefix=PREFIX_0XF3

> now, but also when later running into this commit. If so, I expect
> there'll be a full run through all opcodes to replace the current
> model with the new one?

This is

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8b65b8953af2d49ae1d2d7fcc5b49c5308febbc1

> I'd also like to note that your "Use prefix ... on ..." isn't
> actually correct, as you only changed the non-SSE2AVX encodings.

It should be non-VEX/EVEX/prefix instructions.

> (quoting the actual patch)
>
> >--- a/opcodes/i386-opc.h
> >+++ b/opcodes/i386-opc.h
> >@@ -561,6 +561,16 @@ enum
> > #define VEXW1 2
> > #define VEXWIG        3
> >   VexW,
> >+  /* Regular opcode prefix:
> >+     0: None
> >+     1: Add 0x66 opcode prefix.
> >+     2: Add 0xf2 opcode prefix.
> >+     3: Add 0xf3 opcode prefix.
> >+   */
> >+#define PREFIX_NONE   0
> >+#define PREFIX_0X66   1
> >+#define PREFIX_0XF2   2
> >+#define PREFIX_0XF3   3
> >   /* VEX opcode prefix:
> >      0: VEX 0x0F opcode prefix.
> >      1: VEX 0x0F38 opcode prefix.
> >@@ -575,7 +585,7 @@ enum
> > #define XOP08         3
> > #define XOP09         4
> > #define XOP0A         5
> >-  VexOpcode,
> >+  OpcodePrefix,
>
> I don't think this is a helpful repurposing. Instead I was
> thinking to encode 0F, 0F38, and 0F3A for non-VEX the same way,
> re-using the encodings here. 66, F3, and F2 as a result would
> need separate encoding.

Free feel to change.  It should be easy to change with:

#define Prefix_0X66 OpcodePrefix=PREFIX_0X66
#define Prefix_0XF2 OpcodePrefix=PREFIX_0XF2
#define Prefix_0XF3 OpcodePrefix=PREFIX_0XF3

> I also think it would have been more logical (because of the
> way VEX/XOP/EVEX encode them) if it were
>
> #define PREFIX_NONE     0
> #define PREFIX_66       1
> #define PREFIX_F3       2
> #define PREFIX_F2       3
>

Can you submit a patch?

Thanks.

-- 
H.J.