This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 10/10] x86: correct VFPCLASSP{S,D} operand size handling


On 09.08.2019 16:57,  H.J. Lu  wrote:
On Fri, Aug 9, 2019 at 12:55 AM Jan Beulich <jbeulich@suse.com> wrote:

On 08.08.2019 18:22,  H.J. Lu  wrote:
On Thu, Aug 8, 2019 at 12:47 AM Jan Beulich <JBeulich@suse.com> wrote:

On 07.08.2019 20:00,  H.J. Lu  wrote:
On Wed, Aug 7, 2019 at 1:29 AM Jan Beulich <jbeulich@suse.com> wrote:

On 06.08.2019 23:11,  H.J. Lu  wrote:
On Tue, Aug 6, 2019 at 7:29 AM Jan Beulich <jbeulich@suse.com> wrote:

With AVX512VL disabled (e.g. when writing code for the Knights family
of processors) these insns aren't ambiguous when used with a memory
source, and hence should be accepted without suffix or operand size
specifier. When AVX512VL is enabled, to be consistent with this as
well as other ambiguous operand size handling it seems better to just
wanrn about the ambiguity in AT&T mode, and still default to 512-bit
operands (on the assumption that the code may have been written without
AVX512VL in mind yet).

gas/
2019-08-XX  Jan Beulich  <jbeulich@suse.com>

            * config/tc-i386.c (avx512): New (at file scope), moved from
            (check_VecOperands): ... here.
            (process_suffix): Add [XYZ]MMword operand size handling.
            * testsuite/gas/i386/noavx512-2.s, testsuite/gas/i386/noreg16.s,
            testsuite/gas/i386/noreg32.s, testsuite/gas/i386/noreg64.s: Add
            VFPCLASS tests.
            * testsuite/gas/i386/noavx512-2.l, testsuite/gas/i386/noreg16.d,
            testsuite/gas/i386/noreg16.l, testsuite/gas/i386/noreg32.d,
            testsuite/gas/i386/noreg32.l, testsuite/gas/i386/noreg64.d,
            testsuite/gas/i386/noreg64.l: Adjust expectations.

opcodes/
2019-08-XX  Jan Beulich  <jbeulich@suse.com>

            * i386-opc.tbl (vfpclasspd, vfpclassps): Add Unspecified.
            * i386-tbl.h: Re-generate.


We should keep the suffix even if AVX512VL isn't enabled so that
we don't need to check if AVX512VL isn't enabled to interpret the
instruction.

But that's wrong (and fixing this is the whole point of this patch).
As you've said elsewhere, unambiguous (SIMD in particular) insns
should not require any use of suffixes.


When I look at such instruction, I should be able to tell what it is without
checking if AVX512VL is enabled.

This entirely depends on the context: When all you think about is
Knights hardware (or AVX512F/AVX512DQ in more general terms), then
seeing (and in particular being _forced_ to use) the suffix is
confusing (wrong). No-one says that in this case the suffix won't
be allowed anymore. The problem here seems to be your use of "I",
when instead you should also be considering how other people may
view things.

As a general statement: The assembler should accept (without any
diagnostic) anything that's unambiguously mappable to a valid
encoding.


When I debug assembly codes:

vfpclasspd $0, (%eax), %k0

it is hard to tell the memory operand size.

You're kidding? The presented patch makes no change whatsoever to
the disassembler (as can also be seen from the testsuite extensions
it makes). Doing so would actually be quite hard I think without it
even knowing of the distinction between Knights family and other
processors, and without having any CPU capabilities attribute
attached to insns.


I can compile assembly codes with -g and use gdb to step
through the assembly code:

Breakpoint 2, __strstr_sse2_unaligned ()
     at ../sysdeps/x86_64/multiarch/strstr-sse2-unaligned.S:22
22 movzbl (%rsi), %eax
(gdb) next
23 testb %al, %al
(gdb)

When I see

22   vfpclasspd $0, (%eax), %k0

I can't tell what the memory size is.

Excuse me, but when you go through source code it is assumed
that you know what your source code means and does. No-one
requires you to omit the suffix. But equally no-one should be
required to specify a suffix just to meet your taste.

Let me be frank here: If you continue to refuse to allow this
change in, I'll have to make it work correctly for Intel syntax
mode only (which requires more code for no gain), just to avoid
the need to have you ack the change. I don't think though that
this would be a good course of action.

Jan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]