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

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 06.08.18 at 17:09, <hjl.tools@gmail.com> wrote:
> 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?

I could do that, sure.

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

There's a whole lot of stuff getting added to *pseudos.s.

Jan