Re: [PATCH 02/10] x86: drop stray W

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

On Mon, Aug 12, 2019 at 1:11 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.08.2019 21:12,  H.J. Lu  wrote:
> > On Fri, Aug 9, 2019 at 12:42 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 08.08.2019 17:59,  H.J. Lu  wrote:
> >>> On Wed, Aug 7, 2019 at 8:49 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 07.08.2019 17:13,  H.J. Lu  wrote:
> >>>>> On Wed, Aug 7, 2019 at 12:43 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 06.08.2019 21:37,  H.J. Lu  wrote:
> >>>>>>> On Tue, Aug 6, 2019 at 7:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>
> >>>>>>>> The flag is used to indicate opcodes which can be switched between byte
> >>>>>>>> and word/dword/qword forms (in a "canonical" way). Obviously it's quite
> >>>>>>>> odd then to see it on insns not allowing for byte operands in the first
> >>>>>>>> place. As a result the opcode bytes need to be adjusted accordingly,
> >>>>>>>> which includes comparisons done in optimize_encoding().
> >>>>>>>
> >>>>>>> These encodings do allow byte operand.
> >>>>>>
> >>>>>> By "encodings" I assume you mean the opcodes, not the templates. The
> >>>>>> templates modified here all clearly don't allow byte operands, and
> >>>>>> that's what counts when considering whether W is applicable.
> >>>>>
> >>>>> i.tm.opcode_modifier.w is checked only in process_suffix.  This part
> >>>>>
> >>>>>          /* It's not a byte, select word/dword operation.  */
> >>>>>          if (i.tm.opcode_modifier.w)
> >>>>>            {
> >>>>>              if (i.tm.opcode_modifier.shortform)
> >>>>>                i.tm.base_opcode |= 8;
> >>>>>              else
> >>>>>                i.tm.base_opcode |= 1;
> >>>>>            }
> >>>>>
> >>>>> applies to encoding.  Even if we can't merge entries in i386-opc.tbl,
> >>>>> W still makes senses.   Will keeping W cause any issues?
> >>>>
> >>>> Probably not right now, but I'd have to invest time to re-check the
> >>>> rest of the series without it. But I still don't get it: Other than
> >>>> what you say, W does _not_ make sense when no accepted operand
> >>>> combination allows the if() above to be bypassed. It is an "alter
> >>>> the encoding if the operand is word/dword/qword, i.e. not byte" flag,
> >>>> implying that the encoding should remain unchanged for byte operands,
> >>>> which the templates in question don't accept in the first place.
> >>>>
> >>>> Let me state my position in another way: Every, absolutely every
> >>>> attribute in the templates should have a reason to be there.
> >>>> Everything else should be dropped. Over the last couple of years
> >>>> I've managed to get rid of quite a few pointlessly present
> >>>> attributes. The W flags here are just another example. Not
> >>>> following this fundamental way of handling things has led to the
> >>>> mess that the opcode table was and to a fair degree still is.
> >>>> This is actively hindering maintainability.
> >>>>
> >>>
> >>> W is set on instructions with the operand size encoding bit (w) in SDM.
> >>> It isn't set on anything else.   How will it be a problem?
> >>
> >> "Problem" has to be seen from two perspectives here: There's no
> >> problem with the generated code. But there is a problem in that
> >> people other than you may legitimately wonder why the attribute
> >> is there. I.e. its unnecessary presence is potentially confusing.
> >> Guess how I came to put together this patch?
> >
> > At first, I didn't know why W was used on these instructions.   I
> > found my answer in SDM.   BTW, some other bits can be traced
> > to SDW.
> >
> >> Furthermore at least up until patch 9 the attribute is not used
> >> consistently - it's been missing for MOVSX/MOVZX. Similarly
> >> prior to commit 556059dd13 it hadn't been used for CRC32. Hence
> >> your "its use follows what the SDM says" isn't really applicable.
> >> (Note also how we've recently moved away from the sreg2 / sreg3
> >> distinction the SDM makes.)
> >>
> >
> > W may not be used on all places where it should be.   If we don't
> > want to totally remove W, we could add it where it should be used,
> > remove it where it is used now.
>
> I'm afraid I don't understand: Are you suggesting to cripple the
> code just to be more in line with the SDM? The SDM _may_ be used
> as a guideline, but I think there should be no requirement that
> the code exactly match what the SDM says. Otherwise you'd also
> be suggesting to revert quite a bit of other (more or less
> recent) work of mine. Plus you'd chance to get into trouble if
> SDM (Intel) and PM (AMD) would use differing classification or
> terminology. (I hope it goes without saying that there are no
> grounds for either of the two to be treated "better" than the
> other.)

I just pointed out why W was done this way.

> Instead the code should be written with ease of maintenance in
> mind, which in particular includes having neither unduly many
> templates, nor unduly many/stray attributes on individual
> templates.

I am OK to remove W from these instructions.   But please document
how it should be used so that the next person can understand it.

Thanks.

-- 
H.J.