| Message ID | 20240813111525.2562060-1-torbjorn.svensson@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | testsuite: Avoid running neon test on Cortex-M55 | expand |
I'm not a maintainer but I'd argue the entire test is bogus. The error reporting in this area seems to be somewhat fragile, if you compile it with '-march=armv7-a -mfloat-abi=soft', you also don't get the error this is testing for. I'd argue this kind of user friendly error message should just go through the #include <arm_neon.h> and if a user is using __builtin's directly like this then they better know what they are doing and so 'useful' errors are probably less of a priority. In case you are wondering: no we don't offer nice errors when '#include <arm_neon.h>' is compiled with a MVE enabled combination of march/mcpu, but the errors are somewhat friendlier if you compile a '#include <arm_mve.h>' with a NEON enabled command line. Anyway, lets see what Richard says. On 13/08/2024 12:15, Torbjörn SVENSSON wrote: > Ok for trunk and releases/gcc-14? > > -- > > Cortex-M55 supports VFP, but does not contain neon, so the test is > invalid in this context. > > Without this patch, the following error can be seen in the logs: > > .../attr-neon-builtin-fail2.c: In function 'foo': > .../attr-neon-builtin-fail2.c:13:27: error: implicit declaration of function '__builtin_neon_vaddlsv8qi'; did you mean '__builtin_neon_vabshf'? [-Wimplicit-function-declaration] > .../attr-neon-builtin-fail2.c:13:3: error: cannot convert a value of type 'int' to vector type '__simd128_int16_t' which has different size > > gcc/testsuite/ChangeLog: > > * attr-neon-builtin-fail2.c: Check ET neon. > > Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> > Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com> > --- > gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c > index 9cb5a2ebb90..8942b0d68f1 100644 > --- a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c > +++ b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c > @@ -1,6 +1,7 @@ > /* Check that calling a neon builtin from a function compiled with vfp fails. */ > /* { dg-do compile } */ > /* { dg-require-effective-target arm_vfp_ok } */ > +/* { dg-require-effective-target arm_neon_ok } */ > /* { dg-options "-O2" } */ > /* { dg-add-options arm_vfp } */ >
On 13/08/2024 17:18, Andre Vieira (lists) wrote: > I'm not a maintainer but I'd argue the entire test is bogus. > > The error reporting in this area seems to be somewhat fragile, if you compile it with '-march=armv7-a -mfloat-abi=soft', you also don't get the error this is testing for. I'd argue this kind of user friendly error message should just go through the #include <arm_neon.h> and if a user is using __builtin's directly like this then they better know what they are doing and so 'useful' errors are probably less of a priority. > > In case you are wondering: no we don't offer nice errors when '#include <arm_neon.h>' is compiled with a MVE enabled combination of march/mcpu, but the errors are somewhat friendlier if you compile a '#include <arm_mve.h>' with a NEON enabled command line. > > Anyway, lets see what Richard says. I'm inclined to agree. Unfortunately, we have quite a few suspicious tests: it's the downside of a convention that all patches should be accompanied by tests - if the tests aren't up to much then often cause more problems than they are worth. I'd be happy for this one to go. If you really think it ought to be kept for some reason, then the dg-add-options directive should be updated to match the require effective target (ie 'arm_neon'). R. > > On 13/08/2024 12:15, Torbjörn SVENSSON wrote: >> Ok for trunk and releases/gcc-14? >> >> -- >> >> Cortex-M55 supports VFP, but does not contain neon, so the test is >> invalid in this context. >> >> Without this patch, the following error can be seen in the logs: >> >> .../attr-neon-builtin-fail2.c: In function 'foo': >> .../attr-neon-builtin-fail2.c:13:27: error: implicit declaration of function '__builtin_neon_vaddlsv8qi'; did you mean '__builtin_neon_vabshf'? [-Wimplicit-function-declaration] >> .../attr-neon-builtin-fail2.c:13:3: error: cannot convert a value of type 'int' to vector type '__simd128_int16_t' which has different size >> >> gcc/testsuite/ChangeLog: >> >> * attr-neon-builtin-fail2.c: Check ET neon. >> >> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> >> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com> >> --- >> gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c >> index 9cb5a2ebb90..8942b0d68f1 100644 >> --- a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c >> +++ b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c >> @@ -1,6 +1,7 @@ >> /* Check that calling a neon builtin from a function compiled with vfp fails. */ >> /* { dg-do compile } */ >> /* { dg-require-effective-target arm_vfp_ok } */ >> +/* { dg-require-effective-target arm_neon_ok } */ >> /* { dg-options "-O2" } */ >> /* { dg-add-options arm_vfp } */ >>
diff --git a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c index 9cb5a2ebb90..8942b0d68f1 100644 --- a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c +++ b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c @@ -1,6 +1,7 @@ /* Check that calling a neon builtin from a function compiled with vfp fails. */ /* { dg-do compile } */ /* { dg-require-effective-target arm_vfp_ok } */ +/* { dg-require-effective-target arm_neon_ok } */ /* { dg-options "-O2" } */ /* { dg-add-options arm_vfp } */
Ok for trunk and releases/gcc-14? -- Cortex-M55 supports VFP, but does not contain neon, so the test is invalid in this context. Without this patch, the following error can be seen in the logs: .../attr-neon-builtin-fail2.c: In function 'foo': .../attr-neon-builtin-fail2.c:13:27: error: implicit declaration of function '__builtin_neon_vaddlsv8qi'; did you mean '__builtin_neon_vabshf'? [-Wimplicit-function-declaration] .../attr-neon-builtin-fail2.c:13:3: error: cannot convert a value of type 'int' to vector type '__simd128_int16_t' which has different size gcc/testsuite/ChangeLog: * attr-neon-builtin-fail2.c: Check ET neon. Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com> --- gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c | 1 + 1 file changed, 1 insertion(+)