This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

Jan



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]