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

[Jan Beulich <jbeulich at suse dot com>]

On 24.01.2020 12:51, H.J. Lu 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.

None of the patches I've posted so far has touched movslq.

> 2.  Allow DWORD source with movsxd.

I guess you mean WORD, for Intel64 mode. Which again matches what
my patch does.

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

My patch also adds tests which would have failed before. There are two
problems I see here:
1) Why, instead of reviewing my patch (using less arbitrary special
casing), did you feel the need of creating your own version? (As said
before, I'm glad you did the disassembler side change, so thanks for
that part.)
2) Wouldn't it have been fair to wait with committing your change
until we've settled this discussion? I'll rebase my series on top,
sure, but the way you've done it will merely make me want undo your
assembler change and put in my variant of it. This could have been
had in one single step. And do you realize that it's a change to
Intel syntax behavior only, of which strictly speaking I'm the
maintainer?

Jan