[PATCH 3/6] x86: improve operand reversal

[H.J. Lu]

On Mon, Aug 6, 2018 at 8:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> 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.

Thank for doing 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.
>>>
>>> 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.

That will make sure that {load} and {store} are handler properly
by actually testing them, instead of relying on the deprecated
".s" suffix.

-- 
H.J.