V2 [PATCH] x86: Rename VexOpcode to OpcodePrefix

[Jan Beulich]

On 14.10.2020 14:55, H.J. Lu wrote:
> 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:
>>> * 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

I don't understand your reply, which makes me assume you didn't
understand my question. I was asking whether the choice of
adjusting just xorpd, cvtdq2pd, and cvtpd2dq was arbitrary.

>> 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

Yes, I've seen this in the meantime, thanks. The same will of
course want doing for the VEX etc encoded insns.

>> 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?

Sure. I may be able to find time for this in several months ...

Jan