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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Feb 3, 2020 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.02.2020 15:49, H.J. Lu wrote:
> > On Mon, Feb 3, 2020 at 12:38 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> 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 isn't valid AT&T mnemonic.  No one should use it.
>
> Mind telling me what you derive this from? It is my understanding
> that the general rule of how to derive AT&T mnemonics is to take
> the Intel manual's mnemonic and add a set of suffixes if varying

MOVSXD is a special case.  The typical usages of AT&T syntax are

movslq %eax, %rcx
movslq (%rax), %rcx

Anything else should be MOVSXD without any suffixes.  It is one
less mnemonic we need to apply complex suffix rules.

> operand sizes are permitted. I can accept _new_ insns (in
> particular SIMD ones) to not get suffixes supported when they're
> not really needed. But this is an original x86-64 GPR insn. It
> should be consistent with other original x86-64 GPR insns.
>
> Furthermore, if it really was intentional for your commit to

Yes, it was intentional.

> remove support for the suffix, then why does the description not
> say so (nor, what's perhaps even worse, the ChangeLog entry)?
>

Sorry I missed it.

-- 
H.J.