Re: [PATCH] x86: re-order insn template fields

[Jan Beulich <jbeulich@suse.com>]

On 06.07.2022 19:10, H.J. Lu wrote:
> On Wed, Jul 6, 2022 at 6:43 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> This saves quite a number of shift instructions: The "operands" field
>> can now be retrieved by just masking (no shift), and extracting the
>> "extension_opcode" field now only requires a (signed) right shift, with
>> no prereq left one. (Of course there may be architectures where, in a
>> cross build, there might be no difference at all, e.g. when there are
>> suitable bitfield extraction insns.)
>>
>> --- a/opcodes/i386-gen.c
>> +++ b/opcodes/i386-gen.c
>> @@ -1444,8 +1444,8 @@ output_i386_opcode (FILE *table, const c
>>      fail (_("%s:%d: %s: residual opcode (0x%0*llx) too large\n"),
>>           filename, lineno, name, 2 * length, opcode);
>>
>> -  fprintf (table, "  { \"%s\", 0x%0*llx%s, %s, %lu,\n",
>> -          name, 2 * (int)length, opcode, end, extension_opcode, i);
>> +  fprintf (table, "  { \"%s\", 0x%0*llx%s, %lu, %s,\n",
>> +          name, 2 * (int)length, opcode, end, i, extension_opcode);
>>
>>    process_i386_opcode_modifier (table, opcode_modifier, space, prefix,
>>                                 operand_types, lineno);
>> --- a/opcodes/i386-opc.h
>> +++ b/opcodes/i386-opc.h
>> @@ -929,6 +929,12 @@ typedef struct insn_template
>>     from all other values above.  */
>>  #define Opcode_VexW    0xf /* Operand order controlled by VEX.W. */
>>
>> +  /* how many operands */
>> +  unsigned int operands:3;
>> +
>> +  /* spare bits */
>> +  unsigned int :4;
>> +
> 
> These fields need some comments to explain why they are done
> this way.

Would you mind explaining yourself a little further. "operands", after
all, ...

>>  /* (Fake) base opcode value for pseudo prefixes.  */
>>  #define PSEUDO_PREFIX 0
>>
>> @@ -952,9 +958,6 @@ typedef struct insn_template
>>  #define Prefix_REX             8       /* {rex} */
>>  #define Prefix_NoOptimize      9       /* {nooptimize} */
>>
>> -  /* how many operands */
>> -  unsigned int operands:3;
>> -

... has been exactly the same way before, just in a different position.
IOW I guess I don't really understand what exactly you want to see
described in the comment(s) (which includes it not being clear to me
whether you want each field's comment extended, just one of them, or
a wider comment on the overall bitfield uses here).

Jan