On Tue, Apr 7, 2020 at 2:29 PM Jakub Jelinek <ja...@redhat.com> wrote: > > Hi! > > The following testcases are miscompiled, because expand_vec_perm_pshufb > incorrectly thinks it can use vpshufb instruction for the permutations > when it can't. > The > if (vmode == V32QImode) > { > /* vpshufb only works intra lanes, it is not > possible to shuffle bytes in between the lanes. */ > for (i = 0; i < nelt; ++i) > if ((d->perm[i] ^ i) & (nelt / 2)) > return false; > } > intra-lane check which is correct has been copied and adjusted for 64-byte > modes into: > if (vmode == V64QImode) > { > /* vpshufb only works intra lanes, it is not > possible to shuffle bytes in between the lanes. */ > for (i = 0; i < nelt; ++i) > if ((d->perm[i] ^ i) & (nelt / 4)) > return false; > } > which is not correct, because 64-byte modes have 4 lanes rather than just > two and the above is only testing that the permutation grabs even lane elts > from even lanes and odd lane elts from odd lanes, but not that they are > from the same 256-bit half. > > The following patch fixes it by using 3 * nelt / 4 instead of nelt / 4, > so we actually check the most significant 2 bits rather than just one. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and > release branches? > > 2020-04-07 Jakub Jelinek <ja...@redhat.com> > > PR target/94509 > * config/i386/i386-expand.c (expand_vec_perm_pshufb): Fix the check > for inter-lane permutation for 64-byte modes. > > * gcc.target/i386/avx512bw-pr94509-1.c: New test. > * gcc.target/i386/avx512bw-pr94509-2.c: New test.
The patch is OK, and after reading your detailed explanation, even as obvious everywhere. Thanks, Uros. > --- gcc/config/i386/i386-expand.c.jj 2020-04-07 08:27:13.770891167 +0200 > +++ gcc/config/i386/i386-expand.c 2020-04-07 10:40:09.754089204 +0200 > @@ -16781,7 +16781,7 @@ expand_vec_perm_pshufb (struct expand_ve > /* vpshufb only works intra lanes, it is not > possible to shuffle bytes in between the lanes. */ > for (i = 0; i < nelt; ++i) > - if ((d->perm[i] ^ i) & (nelt / 4)) > + if ((d->perm[i] ^ i) & (3 * nelt / 4)) > return false; > } > } > --- gcc/testsuite/gcc.target/i386/avx512bw-pr94509-1.c.jj 2020-04-07 > 10:42:58.560566492 +0200 > +++ gcc/testsuite/gcc.target/i386/avx512bw-pr94509-1.c 2020-04-07 > 10:44:44.010990594 +0200 > @@ -0,0 +1,30 @@ > +/* PR target/94509 */ > +/* { dg-do run { target avx512bw } } */ > +/* { dg-options "-O2 -mavx512bw" } */ > + > +#define AVX512BW > +#include "avx512f-helper.h" > + > +typedef unsigned short __attribute__ ((__vector_size__ (64))) V; > + > +__attribute__((noipa)) V > +foo (V x) > +{ > + return __builtin_shuffle (x, (V) { 0, 0, 0, 0, 0, 0, 0, 0, > + 15, 15, 15, 15, 15, 15, 15, 15, > + 0, 0, 0, 0, 0, 0, 0, 0, > + 15, 15, 15, 15, 15, 15, 15, 15 }); > +} > + > +static void > +TEST (void) > +{ > + V v = foo ((V) { 1, 2, 3, 4, 5, 6, 7, 8, > + 9, 10, 11, 12, 13, 14, 15, 16, > + 17, 18, 19, 20, 21, 22, 23, 24, > + 25, 26, 27, 28, 29, 30, 31, 32 }); > + unsigned int i; > + for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) > + if (v[i] != ((i & 8) ? 16 : 1)) > + abort (); > +} > --- gcc/testsuite/gcc.target/i386/avx512bw-pr94509-2.c.jj 2020-04-07 > 10:43:01.739518984 +0200 > +++ gcc/testsuite/gcc.target/i386/avx512bw-pr94509-2.c 2020-04-07 > 10:44:49.811903904 +0200 > @@ -0,0 +1,38 @@ > +/* PR target/94509 */ > +/* { dg-do run { target avx512bw } } */ > +/* { dg-options "-O2 -mavx512bw" } */ > + > +#define AVX512BW > +#include "avx512f-helper.h" > + > +typedef unsigned char __attribute__ ((__vector_size__ (64))) V; > + > +__attribute__((noipa)) V > +foo (V x) > +{ > + return __builtin_shuffle (x, (V) { 0, 1, 0, 1, 0, 1, 0, 1, > + 0, 1, 0, 1, 0, 1, 0, 1, > + 30, 31, 30, 31, 30, 31, 30, 31, > + 30, 31, 30, 31, 30, 31, 30, 31, > + 0, 1, 0, 1, 0, 1, 0, 1, > + 0, 1, 0, 1, 0, 1, 0, 1, > + 30, 31, 30, 31, 30, 31, 30, 31, > + 30, 31, 30, 31, 30, 31, 30, 31 }); > +} > + > +static void > +TEST (void) > +{ > + V v = foo ((V) { 1, 2, 3, 4, 5, 6, 7, 8, > + 9, 10, 11, 12, 13, 14, 15, 16, > + 17, 18, 19, 20, 21, 22, 23, 24, > + 25, 26, 27, 28, 29, 30, 31, 32, > + 33, 34, 35, 36, 37, 38, 39, 40, > + 41, 42, 43, 44, 45, 46, 47, 48, > + 49, 50, 51, 52, 53, 54, 55, 56, > + 57, 58, 59, 60, 61, 62, 63, 64 }); > + unsigned int i; > + for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) > + if (v[i] != ((i & 16) ? 31 : 1) + (i & 1)) > + abort (); > +} > > Jakub >