| Message ID | 5733428B.20106@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00842.html Thanks, Kyrill On 11/05/16 15:32, Kyrill Tkachov wrote: > Hi all, > > In this PR a NEON builtin is introduced during SLP vectorisation even when NEON is not available > because arm_builtin_vectorized_function is missing an appropriate check in the BSWAP handling code. > > Then during expand when we try to expand the NEON builtin the code in arm_expand_neon_builtin rightly > throws an error telling the user to enable NEON, even though the testcase doesn't use any intrinsics. > > This patch fixes the bug by bailing out early if !TARGET_NEON. This allows us to remove a redundant > TARGET_NEON check further down in the function as well. > > Bootstrapped and tested on arm-none-linux-gnueabihf. > Ok for trunk? > > This appears on GCC 6 as well. > On older branches the test failure doesn't trigger but the logic looks buggy anyway. > Ok for the branches as well if testing is clean? > > Thanks, > Kyrill > > 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/71056 > * config/arm/arm-builtins.c (arm_builtin_vectorized_function): Return > NULL_TREE early if NEON is not available. Remove now redundant check > in ARM_CHECK_BUILTIN_MODE. > > 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/71056 > * gcc.target/arm/pr71056.c: New test.
On 11/05/16 15:32, Kyrill Tkachov wrote: > Hi all, > > In this PR a NEON builtin is introduced during SLP vectorisation even when NEON is not available > because arm_builtin_vectorized_function is missing an appropriate check in the BSWAP handling code. > > Then during expand when we try to expand the NEON builtin the code in arm_expand_neon_builtin rightly > throws an error telling the user to enable NEON, even though the testcase doesn't use any intrinsics. > > This patch fixes the bug by bailing out early if !TARGET_NEON. This allows us to remove a redundant > TARGET_NEON check further down in the function as well. > > Bootstrapped and tested on arm-none-linux-gnueabihf. > Ok for trunk? > > This appears on GCC 6 as well. > On older branches the test failure doesn't trigger but the logic looks buggy anyway. > Ok for the branches as well if testing is clean? > > Thanks, > Kyrill > > 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/71056 > * config/arm/arm-builtins.c (arm_builtin_vectorized_function): Return > NULL_TREE early if NEON is not available. Remove now redundant check > in ARM_CHECK_BUILTIN_MODE. > > 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/71056 > * gcc.target/arm/pr71056.c: New test. OK. LGTM - please apply if no regressions and backport onto GCC 6 after the auto-testers have let this bake on trunk for a little while. I'd rather not apply it to the release branches unless we can trigger it there but it maybe newer logic in the bswap pass that detects this. regards Ramana
Hi Ramana, On 19/05/16 14:36, Ramana Radhakrishnan wrote: > On 11/05/16 15:32, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR a NEON builtin is introduced during SLP vectorisation even when NEON is not available >> because arm_builtin_vectorized_function is missing an appropriate check in the BSWAP handling code. >> >> Then during expand when we try to expand the NEON builtin the code in arm_expand_neon_builtin rightly >> throws an error telling the user to enable NEON, even though the testcase doesn't use any intrinsics. >> >> This patch fixes the bug by bailing out early if !TARGET_NEON. This allows us to remove a redundant >> TARGET_NEON check further down in the function as well. >> >> Bootstrapped and tested on arm-none-linux-gnueabihf. >> Ok for trunk? >> >> This appears on GCC 6 as well. >> On older branches the test failure doesn't trigger but the logic looks buggy anyway. >> Ok for the branches as well if testing is clean? >> >> Thanks, >> Kyrill >> >> 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/71056 >> * config/arm/arm-builtins.c (arm_builtin_vectorized_function): Return >> NULL_TREE early if NEON is not available. Remove now redundant check >> in ARM_CHECK_BUILTIN_MODE. >> >> 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/71056 >> * gcc.target/arm/pr71056.c: New test. > OK. LGTM - please apply if no regressions and backport onto GCC 6 after the auto-testers have let this bake on trunk for a little while. > > I'd rather not apply it to the release branches unless we can trigger it there but it maybe newer logic in the bswap pass that detects this. Thanks, the patch has been in trunk for a week without any complaints. I'll apply it to GCC 6 next week. As for the other branches, the logic in arm_builtin_vectorized_function there looks vulnerable as well, but I haven't been able to trigger this there. I think this needs certain combinations of bswap and SLP vectorisation improvements to trigger. I've tried writing a few testcases manually to trigger this but was not able to. I'm happy to not apply this to the other branches unless we get a bug report about it. Thanks for the review, Kyrill > > regards > Ramana
diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c index 90fb40fed24cd31ed7f718664fc9b45e58c3cfa8..68b2839879f78e8d819444fbc11d2a91f8d6279a 100644 --- a/gcc/config/arm/arm-builtins.c +++ b/gcc/config/arm/arm-builtins.c @@ -2861,6 +2861,10 @@ arm_builtin_vectorized_function (unsigned int fn, tree type_out, tree type_in) int in_n, out_n; bool out_unsigned_p = TYPE_UNSIGNED (type_out); + /* Can't provide any vectorized builtins when we can't use NEON. */ + if (!TARGET_NEON) + return NULL_TREE; + if (TREE_CODE (type_out) != VECTOR_TYPE || TREE_CODE (type_in) != VECTOR_TYPE) return NULL_TREE; @@ -2875,7 +2879,7 @@ arm_builtin_vectorized_function (unsigned int fn, tree type_out, tree type_in) NULL_TREE is returned if no such builtin is available. */ #undef ARM_CHECK_BUILTIN_MODE #define ARM_CHECK_BUILTIN_MODE(C) \ - (TARGET_NEON && TARGET_FPU_ARMV8 \ + (TARGET_FPU_ARMV8 \ && flag_unsafe_math_optimizations \ && ARM_CHECK_BUILTIN_MODE_1 (C)) diff --git a/gcc/testsuite/gcc.target/arm/pr71056.c b/gcc/testsuite/gcc.target/arm/pr71056.c new file mode 100644 index 0000000000000000000000000000000000000000..136754eb13c4c4f8f840001d5520cf27f3c57461 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr71056.c @@ -0,0 +1,32 @@ +/* PR target/71056. */ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_vfp3_ok } */ +/* { dg-options "-O3 -mfpu=vfpv3" } */ + +/* Check that compiling for a non-NEON target doesn't try to introduce + a NEON vectorized builtin. */ + +extern char *buff; +int f2 (); +struct T1 +{ + int reserved[2]; + unsigned int ip; + unsigned short cs; + unsigned short rsrv2; +}; +void +f3 (const char *p) +{ + struct T1 x; + __builtin_memcpy (&x, p, sizeof (struct T1)); + x.reserved[0] = __builtin_bswap32 (x.reserved[0]); + x.reserved[1] = __builtin_bswap32 (x.reserved[1]); + x.ip = __builtin_bswap32 (x.ip); + x.cs = x.cs << 8 | x.cs >> 8; + x.rsrv2 = x.rsrv2 << 8 | x.rsrv2 >> 8; + if (f2 ()) + { + __builtin_memcpy (buff, "\n", 1); + } +}
Hi all, In this PR a NEON builtin is introduced during SLP vectorisation even when NEON is not available because arm_builtin_vectorized_function is missing an appropriate check in the BSWAP handling code. Then during expand when we try to expand the NEON builtin the code in arm_expand_neon_builtin rightly throws an error telling the user to enable NEON, even though the testcase doesn't use any intrinsics. This patch fixes the bug by bailing out early if !TARGET_NEON. This allows us to remove a redundant TARGET_NEON check further down in the function as well. Bootstrapped and tested on arm-none-linux-gnueabihf. Ok for trunk? This appears on GCC 6 as well. On older branches the test failure doesn't trigger but the logic looks buggy anyway. Ok for the branches as well if testing is clean? Thanks, Kyrill 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/71056 * config/arm/arm-builtins.c (arm_builtin_vectorized_function): Return NULL_TREE early if NEON is not available. Remove now redundant check in ARM_CHECK_BUILTIN_MODE. 2016-05-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/71056 * gcc.target/arm/pr71056.c: New test.