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

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

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.
-- 
H.J.