Re: [PATCH] x86-64: Properly encode and decode movsxd

[Jan Beulich <jbeulich at suse dot com>]

On 26.01.2020 16:00, H.J. Lu wrote:
> On Fri, Jan 24, 2020 at 3:51 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Fri, Jan 24, 2020 at 12:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 23.01.2020 21:24, H.J. Lu wrote:
>>>> movsxd is a 64-bit only instruction.  It supports both 16-bit and 32-bit
>>>> destination registers.  Its AT&T mnemonic is movslq which only supports
>>>> 64-bit destination register.  There is also a discrepancy between AMD64
>>>> and Intel64 on movsxd with 16-bit destination register.  AMD64 supports
>>>> 32-bit source operand and Intel64 supports 16-bit source operand.
>>>>
>>>> This patch updates movsxd encoding and decoding to alow 16-bit and 32-bit
>>>> destination registers.  It also handles movsxd with 16-bit destination
>>>> register for AMD64 and Intel 64.
>>>
>>> I appreciate you taking care of the disassembler and documentation
>>> side, but would you mind pointing out what's wrong with the previously
>>> posted patches making the assembler deal with this correctly? For
>>> example, I get away ...
>>>
>>>> --- a/gas/config/tc-i386.c
>>>> +++ b/gas/config/tc-i386.c
>>>> @@ -6690,7 +6690,9 @@ check_long_reg (void)
>>>>            && i.tm.operand_types[op].bitfield.dword)
>>>>        {
>>>>       if (intel_syntax
>>>> -         && i.tm.opcode_modifier.toqword
>>>> +         && (i.tm.opcode_modifier.toqword
>>>> +             /* Also convert to QWORD for MOVSXD.  */
>>>> +             || i.tm.base_opcode == 0x63)
>>>>           && i.types[0].bitfield.class != RegSIMD)
>>>
>>> ... without such a (pretty arbitrary) adjustment (as an aside: the
>>> comment is slightly misleading in that the change also affects some
>>> MOVSX templates), while ...
>>
>> My patch in assembler
>>
>> 1.  Keep movslq unchanged, which only allows 64-bit destination register.
>> 2.  Allow DWORD source with movsxd.
>>
>> My testcases cover the above.  Without the assembler change, I got
>>
>> FAIL: x86-64 movsxd (AMD64)
>> FAIL: x86-64 movsxd (AMD64) (Intel mode)
>> FAIL: x86-64 movsxd (Intel64)
>> FAIL: x86-64 movsxd (Intel64) (Intel mode)
>>
> 
> I am checking in this patch tomorrow.

And you broke previously working code, which has no testcase so
far, but which
https://sourceware.org/ml/binutils/2019-12/msg00354.html
adds a test for:

movsxdl (%rax),%rcx

This is because you added No_lSuf to the existing template.
Obviously the No_wSuf / No_lSuf present on the two new templates
then are similarly wrong. Just like for MOVSX/MOVZX in AT&T mode
a suffix should be permitted imo (even if there's no ambiguity
here), specifying the size of the source (memory) operand. An
alternative would be to make MOVSX/MOVZX strictly Intel syntax
mode only, but I guess there would then be a fair risk of
breaking existing code.

I need to know which direction to go in order to sensibly rebase
that patch as well as the other remaining ones from that series.

Jan