This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] x86-64: Properly encode and decode movsxd
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Jan Beulich <jbeulich at suse dot com>
- Cc: Binutils <binutils at sourceware dot org>
- Date: Mon, 3 Feb 2020 09:19:04 -0800
- Subject: Re: [PATCH] x86-64: Properly encode and decode movsxd
- References: <20200123202450.12441-1-hjl.tools@gmail.com> <3a2bae0b-19b9-31f9-3665-841802bd0c84@suse.com> <CAMe9rOosYu++bdMhDv4NDOAW4Yi33nRu0TLYdVXxmAC6ObTLVQ@mail.gmail.com> <CAMe9rOr9j4hB1htCG9=PJNtHgPPigntaWNP_jQeFh3QzkcFG+Q@mail.gmail.com> <b6cb7678-9c77-7add-e68a-d0d2d08d4f3c@suse.com> <CAMe9rOqPynD6=MWGX3m+HMeLeM4+iCAAsTrzf=xgmQd5R8S7nQ@mail.gmail.com> <cf9e09f6-1824-c030-f10c-b26eeac77907@suse.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.