Re: [PATCH 3/6] x86: improve operand reversal

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

On Thu, Aug 2, 2018 at 9:05 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 02.08.18 at 17:04, <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 2, 2018 at 7:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 02.08.18 at 14:51, <hjl.tools@gmail.com> wrote:
>>>> On Thu, Aug 2, 2018 at 5:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 02.08.18 at 14:19, <hjl.tools@gmail.com> wrote:
>>>>>> On Wed, Aug 1, 2018 at 11:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> In quite a few cases the .s suffix or {load} / {store} prefixes did not
>>>>>>> work as intended, or produced errors when they're supposed to be ignored
>>>>>>> when it is not possible to carry out the request.
>>>>>>>
>>>>>>> The change here re-purposes(?) the .s suffix to no longer mean "store"
>>>>>>> (if that's what 's' did stand for), since the forms used in the base
>>>>>>> templates are not consistently loads (and we unlikely want to change
>>>>>>> that). The pseudo prefixes will now fulfill what their names say, i.e.
>>>>>>> {load} now only ever produces a load form encoding (if available) while
>>>>>>> {store} only ever produces a store form one (again if available). This
>>>>>>> requires minimal test suite adjustments, while the majority of the
>>>>>>> changes there are simply additions.
>>>>>>>
>>>>>>
>>>>>> I prefer not to change the behavior of the `.s' suffix, unless it is to fix
>>>>>> the wrong encoding.  I don't see the need for the 'swap' pseudo prefix.
>>>>>> If the programmer doesn't care load/store encoding, "swap" isn't really
>>>>>> useful.
>>>>>
>>>>> There's no {swap} prefix. I've just (verbally) assigned the meaning of
>>>>> "swap" to the .s suffix (which is the behavior it always had afaict,
>>>>> rather than forcing a store to be used, as one could have implied from
>>>>> it being 's' and it having had the same behavior as {store}, just that
>>>>> the behavior was clearly wrong for {store}). This isn't a very good
>>>>> association, but I couldn't come up with anything better that would
>>>>> fit the 's'.
>>>>>
>>>>> Please take a look at the testsuite adjustments - this gives a pretty
>>>>> good picture, and you'll notice that uses of .s continue to behave as
>>>>> before. The adjustments (beyond the various additions) were for
>>>>> {load} and/or {store} now behaving according to their names.
>>>>>
>>>>
>>>> There are many test changes,  Can you list a couple for before and
>>>> after comparison?
>>>
>>> I'm afraid I don't really understand what you're after. Looking at the
>>> patch makes pretty obvious what the very few changes are that aren't
>>> plain additions. Iirc there were only two mov-es with {load} (or was
>>> it {store}) prefix where the output changed.
>>
>> I prefer not to make any changes for the `s' prefix.  It is very hard to
>> tell what changed in disassembler output since your patch removes
>> and adds a large chunk of disassembler output.
>
> Where? There is a total of two lines each replaced in
> testsuite/gas/i386/pseudos.d and testsuite/gas/i386/x86-64-pseudos.d.
> For the other removal - there's none really, I'm only re-using other .d
> files instead of replicating the additions:
>
>         testsuite/gas/i386/ilp32/x86-64-opts.d,
>         testsuite/gas/i386/ilp32/x86-64-opts-intel.d,
>         testsuite/gas/i386/ilp32/x86-64-sse2avx-opts.d,
>         testsuite/gas/i386/ilp32/x86-64-sse2avx-opts-intel.d: Refer to
>         non-ILP32 output.
>
> It is entirely unclear to me why, if we already have to have tests which
> are entirely indifferent between ILP32 and LP64, these tests hadn't
> been written without such redundancy in the first place. I'm not really
> eager to do so, but I could of course split out that part of the change.

I deprecated ".s", ".d8" and ".d32" when I added {load} and {store} prefixes:

commit 86fa6981e7487e2c2df4337aa75ed2d93c32eaf2
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Thu Mar 9 09:58:46 2017 -0800

    X86: Add pseudo prefixes to control encoding

    Many x86 instructions have more than one encodings.  Assembler picks
    the default one, usually the shortest one.  Although the ".s", ".d8"
    and ".d32" suffixes can be used to swap register operands or specify
    displacement size, they aren't very flexible.  This patch adds pseudo
    prefixes, {xxx}, to control instruction encoding.  The available
    pseudo prefixes are {disp8}, {disp32}, {load}, {store}, {vex2}, {vex3}
    and {evex}.  Pseudo prefixes are preferred over the ".s", ".d8" and
    ".d32" suffixes, which are deprecated.

    gas/

            * config/tc-i386.c (_i386_insn): Add dir_encoding and
            vec_encoding.  Remove swap_operand and need_vrex.
            (extra_symbol_chars): Add '}'.
            (md_begin): Mark '}' with LEX_BEGIN_NAME.  Allow '}' in
            mnemonic.
            (build_vex_prefix): Don't use 2-byte VEX encoding with
            {vex3}.  Check dir_encoding and load.
            (parse_insn): Check pseudo prefixes.  Set dir_encoding.
            (VEX_check_operands): Likewise.
            (match_template): Check dir_encoding and load.
            (parse_real_register): Set vec_encoding instead of need_vrex.
            (parse_register): Likewise.
            * doc/c-i386.texi: Document {disp8}, {disp32}, {load}, {store},
            {vex2}, {vex3} and {evex}.  Remove ".s", ".d8" and ".d32"

Please don't make any changes to the deprecated ".s".

-- 
H.J.