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

[H.J. Lu]

On Mon, Jan 27, 2020 at 8:17 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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.

I meant DWORD for AMD64.

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

Your patches check opcode.

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

Please submit a single combined patch to address:

https://sourceware.org/bugzilla/show_bug.cgi?id=25438

-- 
H.J.