[PATCH] x86: re-order insn template fields

[PATCH] x86: re-order insn template fields

[Jan Beulich]

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;
+
 /* (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;
-
   /* the bits in opcode_modifier are used to generate the final opcode from
      the base_opcode.  These bits also are used to detect alternate forms of
      the same instruction */

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

[H.J. Lu]

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.

>  /* (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;
> -
>    /* the bits in opcode_modifier are used to generate the final opcode from
>       the base_opcode.  These bits also are used to detect alternate forms of
>       the same instruction */



-- 
H.J.

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

[Jan Beulich]

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

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

[H.J. Lu]

On Wed, Jul 6, 2022 at 11:47 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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).

Please add some comments to describe the motivation of this change.
Your commit log is nice.

Thanks.

-- 
H.J.