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

["H.J. Lu" <hjl.tools@gmail.com>]

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.