Bug 24434 - Missing testsuite coverage for AVX512F gathers (and scatters?) with no base register
Summary: Missing testsuite coverage for AVX512F gathers (and scatters?) with no base r...
Status: RESOLVED DUPLICATE of bug 23465
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.33
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-10 09:01 UTC by Jakub Jelinek
Modified: 2019-04-10 15:09 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2019-04-10 09:01:09 UTC
As mentioned in http://gcc.gnu.org/PR90028 while a gas bug has been fixed since 2.31, I couldn't find any gas/testsuite/i386/ testsuite coverage for (,%[xyz]mm*,*) or disp(,%[xyz]mm*,*) VSIB addressing even on binutils trunk.
Comment 1 Martin Liška 2019-04-10 09:12:46 UTC
Fixed in bintuils with:

commit 629cfaf1b0fbb32a985607c774bd8e7870b9fa94 (HEAD, refs/bisect/bad)
Author: Jan Beulich <jbeulich@novell.com>
Date:   Mon Jul 30 17:25:05 2018 +0200

    x86: don't mistakenly scale non-8-bit displacements
    
    In commit b5014f7af2 I've removed (instead of replaced) a conditional,
    resulting in addressing forms not allowing 8-bit displacements to now
    get their displacements scaled under certain circumstances. Re-add the
    missing conditional.

Minimal reproducer:

$ cat min.s
.text
foo:
	vpgatherqq	8(,%ymm1,1), %ymm0{%k2}

$ ./gas/as-new --64 min.s -o avx512.o && ./binutils/objdump -S avx512.o

avx512.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <foo>:
   0:	62 f2 fd 2a 91 04 0d 	vpgatherqq 0x1(,%ymm1,1),%ymm0{%k2}
   7:	01 00 00 00
Comment 2 Jan Beulich 2019-04-10 09:29:58 UTC
(In reply to Martin Liška from comment #1)
> Fixed in bintuils with:
> 
> commit 629cfaf1b0fbb32a985607c774bd8e7870b9fa94 (HEAD, refs/bisect/bad)
> Author: Jan Beulich <jbeulich@novell.com>
> Date:   Mon Jul 30 17:25:05 2018 +0200
> 
>     x86: don't mistakenly scale non-8-bit displacements

I don't understand this comment: Said commit does not add any S/G test case(s) o the testsuite. I don't think you should have copied the respective gcc bug comment here.
Comment 3 Martin Liška 2019-04-10 10:40:46 UTC
(In reply to Jan Beulich from comment #2)
> (In reply to Martin Liška from comment #1)
> > Fixed in bintuils with:
> > 
> > commit 629cfaf1b0fbb32a985607c774bd8e7870b9fa94 (HEAD, refs/bisect/bad)
> > Author: Jan Beulich <jbeulich@novell.com>
> > Date:   Mon Jul 30 17:25:05 2018 +0200
> > 
> >     x86: don't mistakenly scale non-8-bit displacements
> 
> I don't understand this comment: Said commit does not add any S/G test
> case(s) o the testsuite. I don't think you should have copied the respective
> gcc bug comment here.

Yes, I should have mentioned that the commit fixes the problem and that it would be nice to add the assembly snippet to test suite.
Comment 4 Jakub Jelinek 2019-04-10 10:52:29 UTC
Well, ideally not just that, but much more.
grep 'gather.*(,' gas/testsuite/gas/i386/*.s
shows those VEX encoded ones testing this (in AT&T mode), so perhaps just copy and tweak all or big part of the
grep '\(gather\|scatter\).*(.*{' gas/testsuite/gas/i386/*.s
tests and remove the base register in those (ditto for Intel mode).
(, has EVEX coverage only in the invalid tests, not the valid ones.
Comment 5 H.J. Lu 2019-04-10 14:06:44 UTC
Dup.

*** This bug has been marked as a duplicate of bug 23465 ***
Comment 6 Jakub Jelinek 2019-04-10 14:15:02 UTC
It is not a dup, this PR is about missing testsuite coverage, which is still the case on binutils trunk.
Comment 7 H.J. Lu 2019-04-10 14:45:43 UTC
(In reply to Jakub Jelinek from comment #6)
> It is not a dup, this PR is about missing testsuite coverage, which is still
> the case on binutils trunk.

If you take out the fix, some tests will fail:

FAIL: ix86 EVEX no disp scaling
FAIL: x86-64 EVEX no disp scaling

commit 629cfaf1b0fbb32a985607c774bd8e7870b9fa94
Author: Jan Beulich <jbeulich@novell.com>
Date:   Mon Jul 30 17:25:05 2018 +0200

    x86: don't mistakenly scale non-8-bit displacements
    
    In commit b5014f7af2 I've removed (instead of replaced) a conditional,
    resulting in addressing forms not allowing 8-bit displacements to now
    get their displacements scaled under certain circumstances. Re-add the
    missing conditional.

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 3f5c4e5638..8f77c3d448 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,12 @@
+2018-07-30  Jan Beulich  <jbeulich@suse.com>
+
+  PR gas/23465
+  * config/tc-i386.c (output_disp): Restrict scaling.
+  * testsuite/gas/i386/evex-no-scale.s,
+    testsuite/gas/i386/evex-no-scale-32.d
+    testsuite/gas/i386/evex-no-scale-64.d: New.
+  * testsuite/gas/i386/i386.exp: Run new tests.
Comment 8 H.J. Lu 2019-04-10 14:50:06 UTC
The problem is EVEX disp8 without base register, not VSIB.
Comment 9 Jakub Jelinek 2019-04-10 14:51:49 UTC
Yes, but none of those tests test the VSIB addressing.
We do have AVX2 tests for no base register, why not have also AVX512 VSIB tests?
Comment 10 H.J. Lu 2019-04-10 15:09:30 UTC
(In reply to Jakub Jelinek from comment #9)
> Yes, but none of those tests test the VSIB addressing.
> We do have AVX2 tests for no base register, why not have also AVX512 VSIB
> tests?

I don't think this adds additional test coverage to assembler.