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

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

On Mon, Aug 6, 2018 at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.08.18 at 14:54, <hjl.tools@gmail.com> wrote:
>> On Sun, Aug 5, 2018 at 11:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 03.08.18 at 18:14, <hjl.tools@gmail.com> wrote:
>>>> On Fri, Aug 3, 2018 at 9:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 03.08.18 at 17:56, <hjl.tools@gmail.com> wrote:
>>>>>> On Fri, Aug 3, 2018 at 8:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>> On 03.08.18 at 17:30, <hjl.tools@gmail.com> wrote:
>>>>>>>> On Fri, Aug 3, 2018 at 12:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>>> On 02.08.18 at 18:43, <hjl.tools@gmail.com> wrote:
>>>>>>>>>> Please don't make any changes to the deprecated ".s".
>>>>>>>>>
>>>>>>>>> Excuse me, but how many more times should I state that I don't
>>>>>>>>> make any changes to its behavior? I solely make {store} no longer
>>>>>>>>> match .s in behavior. In fact I've specifically undone the change
>>>>>>>>> to .s, in anticipation of your objection to any adjustment to it.
>>>>>>>>
>>>>>>>> Why do you change testcases of the .s suffix?
>>>>>>>
>>>>>>> s/change/add to/
>>>>>>>
>>>>>>> Deprecated or no, I think the .s suffix should still work, including
>>>>>>> not causing assembly to fail when used. Try assembling the
>>>>>>> additions without the source adjustments, and I think you'll find
>>>>>>> some will fail. To me such adjustments are not "changes to its
>>>>>>
>>>>>> Which one?
>>>>>
>>>>> The additions to *opts.s. For example, this
>>>>>
>>>>>         mov 0x12345678, %eax
>>>>>         mov.s 0x12345678, %eax
>>>>>         mov %eax, 0x12345678
>>>>>         mov.s %eax, 0x12345678
>>>>>         mov 0x123456789abcdef0, %eax
>>>>>         mov.s 0x123456789abcdef0, %eax
>>>>>         mov %eax, 0x123456789abcdef0
>>>>>         mov.s %eax, 0x123456789abcdef0
>>>>>         movabs 0x123456789abcdef0, %eax
>>>>>         movabs.s 0x123456789abcdef0, %eax
>>>>>         movabs %eax, 0x123456789abcdef0
>>>>>         movabs.s %eax, 0x123456789abcdef0
>>>>>         mov %eax, (%rdi)
>>>>>         mov.s %eax, (%rdi)
>>>>>         mov (%rdi), %eax
>>>>>         mov.s (%rdi), %eax
>>>>>         mov %cr0, %rax
>>>>>         mov.s %cr0, %rax
>>>>>         mov %rax, %cr7
>>>>>         mov.s %rax, %cr7
>>>>>         mov %dr0, %rax
>>>>>         mov.s %dr0, %rax
>>>>>         mov %rax, %dr7
>>>>>         mov.s %rax, %dr7
>>>>>
>>>>> doesn't assemble with 2.31.1 (several "unsupported instruction"
>>>>> and one "operand size mismatch").
>>>>
>>>> I saw
>>>>
>>>> s.s:4: Error: unsupported instruction `mov'
>>>> s.s:6: Error: unsupported instruction `mov'
>>>> s.s:10: Error: operand size mismatch for `movabs'
>>>> s.s:14: Error: unsupported instruction `mov'
>>>> s.s:18: Error: unsupported instruction `mov'
>>>> s.s:22: Error: unsupported instruction `mov'
>>>>
>>>> There is no need to fix these since the ".s" suffix has been deprecated.
>>>
>>> "There is no need" can mean several things:
>>> 1) You want me to deliberately remove the code correction (which
>>> may result in overall less readable / maintainable code).
>>> 2) You want me to simply not add tests for the corrected behavior
>>> (which would be contrary to your general position that ideally any
>>> code change would be accompanied by a test).
>>> 3) I can leave the patch the way it is, as it also doesn't mean the
>>> behavior must not be fixed.
>>> And perhaps more.
>>
>> I don't want to make any changes to assembler, whose sole purposes
>> are to change/improve the behavior of the ".s"  suffix.
>>
>>> Based on what I've written before, I'm opposed to 1, I could live
>>> with 2, but I'd much prefer 3.
>>>
>>> A further note on deprecation of these suffixes: By looking at just
>>> the source code, how did you expect me to know they're deprecated?
>>
>> Can you recommend a good way to indicate that?
>
> A code comment in the section responsible for the parsing of these
> suffixes?

Do you care enough to submit a patch?

>>> There was no comment whatsoever added to that effect back when
>>> you've made that change, neither to the .c file nor to the respective
>>> test cases (which, if you suggest to go with 2, should perhaps be
>>> removed altogether).
>>
>> I have removed the ".s" suffix from the assembler manual, but kept
>> the testcases to avoid breaking existing codes.  But we shouldn't
>> make further changes to improve it.
>
> But you realize that the fixing of .s is more a byproduct of fixing
> {load} and {store}? That's why I keep saying that _not_ fixing
> .s at the same time would likely result in worse (and hence harder
> to maintain) code. And that's also why I'm considering option 2
> above tolerable, albeit not ideal.
>

But there is no indication at all in your patch to show that it does
anything remotely to {load} nor {store}.   All your testcase changes
are for the ".s" suffix.

-- 
H.J.