Re: [PATCH v3 4/7] x86-64: further re-work insn/suffix recognition to also cover MOVSL

[Jan Beulich <jbeulich@suse.com>]

On 19.10.2022 23:46, H.J. Lu wrote:
> On Tue, Oct 18, 2022 at 11:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.10.2022 23:48, H.J. Lu wrote:
>>> On Mon, Oct 17, 2022 at 11:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 18.10.2022 00:36, H.J. Lu wrote:
>>>>> On Mon, Oct 17, 2022 at 12:02 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 14.10.2022 19:07, H.J. Lu wrote:
>>>>>>> On Fri, Oct 14, 2022 at 12:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 13.10.2022 19:00, H.J. Lu wrote:
>>>>>>>>> On Wed, Oct 12, 2022 at 11:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 12.10.2022 19:10, H.J. Lu wrote:
>>>>>>>>>>> On Wed, Oct 12, 2022 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 11.10.2022 19:44, H.J. Lu wrote:
>>>>>>>>>>>>> On Wed, Oct 5, 2022 at 12:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> PR gas/29524
>>>>>>>>>>>>>> In order to make MOVSL{,Q} behave similarly to MOVSB{W,L,Q} and
>>>>>>>>>>>>>> MOVSW{L,Q} we need to defer parse_insn()'s emitting of errors unrelated
>>>>>>>>>>>>>> to prefix parsing. Utilize i.error just like match_template() does.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Since movs{b,w,l,q} are string instructions, integer sign extensions
>>>>>>>>>>>>> require a suffix to specify the destination size.  This is different from other
>>>>>>>>>>>>> integer instructions.  Since only the new assembler allows the implicit suffix,
>>>>>>>>>>>>> it won't be easy to use.  We should improve error messages, but allowing
>>>>>>>>>>>>> new syntax doesn't help much.
>>>>>>>>>>>>
>>>>>>>>>>>> It is an earlier change making most of this consistent with MOVZ*; it is
>>>>>>>>>>>
>>>>>>>>>>> MOVZ is different.  There are no MOVZ string instructions.  MOVS has
>>>>>>>>>>> different meanings in ISA.   MOVS difference from MOVZ in assembly
>>>>>>>>>>> syntax should be expected.
>>>>>>>>>>
>>>>>>>>>> You've said so before, yes, but I continue to disagree. And as we can see
>>>>>>>>>> from the series things can be made work consistently (and imo nothing else
>>>>>>>>>> should have been done right from the beginning).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are inconsistencies in ISA.
>>>>>>>>
>>>>>>>> Sure. But we shouldn't add further ones in the assembler.
>>>>>>>
>>>>>>> Assembler just follows ISA.  Programmers should learn to
>>>>>>> deal with it or use a compiler.
>>>>>>
>>>>>> This is entirely non-constructive. Assembler writers should get things into
>>>>>> usable (read: consistent) shape. Plus what ISA are you talking about here?
>>>>>
>>>>> GNU assembler has been this way for a long time and the current GNU
>>>>> assembler will still be in use for the next few years.  Assembler writers
>>>>> should know about all these.
>>>>
>>>> Hmm, so after all not any ISA to follow? Plus do you suggest there's
>>>> only people having written x86 assembler code for many years? And
>>>> there's no people who would prefer to get their code into more
>>>> consistent shape, but who are limited by assembler shortcomings?
>>>
>>> I prefer consistency with existing assemblers and ISA specs.
>>> For new instructions, suffixes should be used only when there is
>>> an ambiguity.  For existing instructions, to support existing assembly
>>> codes, suffixes may be optional even when there is an ambiguity.
>>> But for integer MOVS instructions, suffixes have always been required.
>>> I don't think we should change them now.  We can improve documentation
>>> if needed.
>>>
>>>> In any event, ...
>>>>
>>>>>> We're talking of mnemonics which aren't spelled out in any ISA document
>>>>>> anyway. The only halfway official AT&T doc I'm aware of doesn't provide
>>>>>> room for omitting size suffixes [1]. Yet that's a fundamental feature of gas,
>>>>>> and elsewhere (recently: CMPccXADD) you're even suggesting to force people
>>>>>> to omit suffixes (plus you've previously objected to the disassembler to
>>>>>> consistently emit them in suffix-always mode).
>>>>>>
>>>>>> That same document was also only updated to cover 64-bit code in a half-
>>>>>> hearted way, so can't necessarily be used for 64-bit only insns (it doesn't
>>>>>> list any form of MOVSXD at all afaics, for example). Where not explicitly
>>>>>> mentioned, their intended handling can only be inferred by using analogies.
>>>>>> Nor do we support some of the odd (quirky I would say) mnemonics that are
>>>>>> listed there, like xchglA or movabsbA (which is even wrongly described as
>>>>>> moving an immediate value into the register).
>>>>>>
>>>>>> Bottom line: May I please ask that you take another (constructive) look at
>>>>>> the v4 submission?
>>>>
>>>> ... this request continues to stand.
>>>
>>> I think we should improve diagnostics and documentation, not add new
>>> syntaxes to existing instructions.
>>
>> I continue to disagree, but to make at least some forward progress: Which
>> parts of this series can I expect an okay for (patches 5-7 already have
>> one, but can't go in ahead)? I would try to re-order the series then to
>> put the controversial patch(es) at the end, such that at least parts can
>> go in and I can make further progress with other work. But there's no
>> promise this re-ordering actually is going to work out if it's more than
>> just this one patch which you continue to object to.
>>
> 
> Please submit a new patch set without MOVS changes.

As said - I'll put those in a separate patch at the end of the series. Not
the least because you'll be a little disappointed: The changes to tc-i386.c
simply need to move to another patch then. Hence there's not going to be
much left to make move-with-sign-extend consistent in that final patch
(most of it is then testsuite adjustments/additions).

Jan