[PATCH 3/6] x86: improve operand reversal

[H.J. Lu]

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?

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

-- 
H.J.