Re: [PATCH] x86: infer No_*Suf from other insn attributes

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

On Mon, Nov 14, 2022 at 11:27 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.11.2022 00:33, H.J. Lu wrote:
> > On Mon, Nov 14, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> --- a/opcodes/i386-opc.tbl
> >> +++ b/opcodes/i386-opc.tbl
> >> @@ -75,12 +75,17 @@
> >>  #define Size32 Size=SIZE32
> >>  #define Size64 Size=SIZE64
> >>
> >> +#define IsPrefix IsPrefix|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf
> >> +
> >
> > I prefer to add
> >
> > #define No_Suf No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf
> >
> > to cover more templates.
>
> Iirc you said so two years ago already in the context of "x86: imply
> all No_*Suf when none is set in a template". Yet as before I don't
> like going that route, as that still leaves clutter on the respective
> lines (even if it's less clutter then). Plus the ultimate goal, as
> also said back then, ought to be to move from negative to positive
> forms. Doing things the way done here will avoid touching all those
> lines again which are being touched here.
>
> As a compromise I'd accept introducing NoSuf (or No_Suf) in addition
> to the changes done here, for use on applicable lines not touched
> here already, and for use in the #define-s I'm adding. I'd prefer
> this to be a separate, subsequent patch though (to limit patch size,
> focusing on one transformation at a time. (I could introduce the new
> macro in a prereq patch, using it for only AddrPrefixOpReg right away,
> then have the patch here use it in the new macros, and finally add one
> to use the new macro on the remaining applicable templates.)

An explicit NoSuf (or No_Suf) is better.

> >> @@ -125,6 +130,11 @@
> >>  #define VecSIB512 SIB=VECSIB512
> >>  #define Sibmem SIB=SIBMEM|Modrm
> >>
> >> +#define SIB        No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|SIB
> >
> > Where is this used?
>
> Half of the uses are even visible in patch context of this very hunk.
> The other two are immediately ahead, just outside of patch context.
>
> One more general request, which is particularly relevant on a large
> patch like this one: Can you please trim reply context? Below here
> you did leave over a thousand lines of patch content, leaving me and
> any potential other reader to scroll through to see whether there's
> any further comment.
>

Sure.



-- 
H.J.