diff mbox series

[V2,9/11] Update tests to work with architecture flags changes.

Message ID Zy5so3JAV7ARmq_5@cowardly-lion.the-meissners.org
State New
Headers show
Series Separate PowerPC archiecture bits from ISA flags that use command line options | expand

Commit Message

Michael Meissner Nov. 8, 2024, 7:55 p.m. UTC 
Two tests used -mvsx to raise the processor level to at least power7.  These
tests were rewritten to add cpu=power7 support.

I have built both big endian and little endian bootstrap compilers and there
were no regressions.

In addition, I constructed a test case that used every archiecture define (like
_ARCH_PWR4, etc.) and I also looked at the .machine directive generated.  I ran
this test for all supported combinations of -mcpu, big/little endian, and 32/64
bit support.  Every single instance generated exactly the same code with the
patches installed compared to the compiler before installing the patches.

Can I install this patch on the GCC 15 trunk?

2024-11-06  Michael Meissner  <meissner@linux.ibm.com>

gcc/testsuite/

	* gcc.target/powerpc/ppc-target-4.c: Rewrite the test to add cpu=power7
	when we need to add VSX support.  Add test for adding cpu=power7 no-vsx
	to generate only Altivec instructions.
	* gcc.target/powerpc/pr115688.c: Add cpu=power7 when requesting VSX
	instructions.
---
 .../gcc.target/powerpc/ppc-target-4.c         | 38 ++++++++++++++-----
 gcc/testsuite/gcc.target/powerpc/pr115688.c   |  3 +-
 2 files changed, 31 insertions(+), 10 deletions(-)

Comments

Peter Bergner Nov. 15, 2024, 12:47 a.m. UTC | #1 
On 11/8/24 1:55 PM, Michael Meissner wrote:
> Two tests used -mvsx to raise the processor level to at least power7.  These
> tests were rewritten to add cpu=power7 support.

Again, this cleanup patch like the TARGET_<FOO> -> TARGET_<CPU> patches
is independent of the main patches in this series (ie, patche 1-3),
so let's pull this out of the series and just mention they're cleanup
patches preparing for the actual 3 patch series to come later.



>  /* { dg-skip-if "" { powerpc*-*-darwin* } } */
>  /* { dg-require-effective-target powerpc_fprs } */
>  /* { dg-options "-O2 -ffast-math -mdejagnu-cpu=power5 -mno-altivec -mabi=altivec -fno-unroll-loops" } */
> -/* { dg-final { scan-assembler-times "vaddfp" 1 } } */
> +/* { dg-final { scan-assembler-times "vaddfp" 2 } } */
>  /* { dg-final { scan-assembler-times "xvaddsp" 1 } } */
>  /* { dg-final { scan-assembler-times "fadds" 1 } } */
>  
> @@ -18,10 +18,6 @@
>  #error "__VSX__ should not be defined."
>  #endif
>  
> -#pragma GCC target("altivec,vsx")
> -#include <altivec.h>
> -#pragma GCC reset_options
> -
>  #pragma GCC push_options
>  #pragma GCC target("altivec,no-vsx")

Is this illegal?  We're using -mcpu=power5, which should always flag
an error if we use it with -maltivec or -mvsx.  Isn't that what's
happening above (before your patch too) by the pragma adding -maltivec
to the compile options?  Or does the pragma target through out all
all of our dg-options?  If so, aren't we using the default -mcpu=
values (Power4 for BE and Power8 for LE) which would seem ok on LE,
but a problem on BE.






> -#pragma GCC target("vsx")
> +/* cpu=power7 must be used to enable VSX.  */
> +#pragma GCC target("cpu=power7,vsx")

Is there a reason you're adding -mvsx too, since the -mcpu=power7
should enable VSX implicitly. ...or does the -mno-altivec -mno-vsx
in the dg-options stick around so we beed to override them with
the explicit -mvsx?



>    for (i = 0; i < n; i++)
> -    a[i] = vec_add (b[i], c[i]);
> +    a[i] = b[i] + c[i];

Much better, thanks!  I dislike it when people use vector intrinsics
when straight C code is cleaner and easier to read.





> diff --git a/gcc/testsuite/gcc.target/powerpc/pr115688.c b/gcc/testsuite/gcc.target/powerpc/pr115688.c
> index 5222e66ef17..00c7c301436 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr115688.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr115688.c
> @@ -7,7 +7,8 @@
>  
>  /* Verify there is no ICE under 32 bit env.  */
>  
> -__attribute__((target("vsx")))
> +/* cpu=power7 must be used to enable VSX.  */
> +__attribute__((target("cpu=power7,vsx")))
>  int test (void)
>  {
>    return 0;

Same question as above.  Why the need for adding -mvsx here?

Peter
Michael Meissner Nov. 17, 2024, 8:25 a.m. UTC | #2 
On Thu, Nov 14, 2024 at 06:47:58PM -0600, Peter Bergner wrote:
> On 11/8/24 1:55 PM, Michael Meissner wrote:
> > Two tests used -mvsx to raise the processor level to at least power7.  These
> > tests were rewritten to add cpu=power7 support.
> 
> Again, this cleanup patch like the TARGET_<FOO> -> TARGET_<CPU> patches
> is independent of the main patches in this series (ie, patche 1-3),
> so let's pull this out of the series and just mention they're cleanup
> patches preparing for the actual 3 patch series to come later.

See my latest round of patches.

> 
> >  /* { dg-skip-if "" { powerpc*-*-darwin* } } */
> >  /* { dg-require-effective-target powerpc_fprs } */
> >  /* { dg-options "-O2 -ffast-math -mdejagnu-cpu=power5 -mno-altivec -mabi=altivec -fno-unroll-loops" } */
> > -/* { dg-final { scan-assembler-times "vaddfp" 1 } } */
> > +/* { dg-final { scan-assembler-times "vaddfp" 2 } } */
> >  /* { dg-final { scan-assembler-times "xvaddsp" 1 } } */
> >  /* { dg-final { scan-assembler-times "fadds" 1 } } */
> >  
> > @@ -18,10 +18,6 @@
> >  #error "__VSX__ should not be defined."
> >  #endif
> >  
> > -#pragma GCC target("altivec,vsx")
> > -#include <altivec.h>
> > -#pragma GCC reset_options
> > -
> >  #pragma GCC push_options
> >  #pragma GCC target("altivec,no-vsx")
> 
> Is this illegal?  We're using -mcpu=power5, which should always flag
> an error if we use it with -maltivec or -mvsx.  Isn't that what's
> happening above (before your patch too) by the pragma adding -maltivec
> to the compile options?  Or does the pragma target through out all
> all of our dg-options?  If so, aren't we using the default -mcpu=
> values (Power4 for BE and Power8 for LE) which would seem ok on LE,
> but a problem on BE.

I'm not sure what the question is.  Without the patch as I said, if you use
-mvsx or #pragma GCC target("vsx"), it essentially sets the cpu to at least
power7.  The patch is to make it illega to use -mvsx to raise the cpu level.

> 
> > -#pragma GCC target("vsx")
> > +/* cpu=power7 must be used to enable VSX.  */
> > +#pragma GCC target("cpu=power7,vsx")
> 
> Is there a reason you're adding -mvsx too, since the -mcpu=power7
> should enable VSX implicitly. ...or does the -mno-altivec -mno-vsx
> in the dg-options stick around so we beed to override them with
> the explicit -mvsx?

I was just trying to make the test case work.  I just added the cpu=power7 into
the pragma.
> 
> 
> >    for (i = 0; i < n; i++)
> > -    a[i] = vec_add (b[i], c[i]);
> > +    a[i] = b[i] + c[i];
> 
> Much better, thanks!  I dislike it when people use vector intrinsics
> when straight C code is cleaner and easier to read.
> 
> 
> 
> 
> 
> > diff --git a/gcc/testsuite/gcc.target/powerpc/pr115688.c b/gcc/testsuite/gcc.target/powerpc/pr115688.c
> > index 5222e66ef17..00c7c301436 100644
> > --- a/gcc/testsuite/gcc.target/powerpc/pr115688.c
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr115688.c
> > @@ -7,7 +7,8 @@
> >  
> >  /* Verify there is no ICE under 32 bit env.  */
> >  
> > -__attribute__((target("vsx")))
> > +/* cpu=power7 must be used to enable VSX.  */
> > +__attribute__((target("cpu=power7,vsx")))
> >  int test (void)
> >  {
> >    return 0;
> 
> Same question as above.  Why the need for adding -mvsx here?
> 
> Peter
> 
> 
>
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/powerpc/ppc-target-4.c b/gcc/testsuite/gcc.target/powerpc/ppc-target-4.c
index feef76db461..5e2ecf34f24 100644
--- a/gcc/testsuite/gcc.target/powerpc/ppc-target-4.c
+++ b/gcc/testsuite/gcc.target/powerpc/ppc-target-4.c
@@ -2,7 +2,7 @@ 
 /* { dg-skip-if "" { powerpc*-*-darwin* } } */
 /* { dg-require-effective-target powerpc_fprs } */
 /* { dg-options "-O2 -ffast-math -mdejagnu-cpu=power5 -mno-altivec -mabi=altivec -fno-unroll-loops" } */
-/* { dg-final { scan-assembler-times "vaddfp" 1 } } */
+/* { dg-final { scan-assembler-times "vaddfp" 2 } } */
 /* { dg-final { scan-assembler-times "xvaddsp" 1 } } */
 /* { dg-final { scan-assembler-times "fadds" 1 } } */
 
@@ -18,10 +18,6 @@ 
 #error "__VSX__ should not be defined."
 #endif
 
-#pragma GCC target("altivec,vsx")
-#include <altivec.h>
-#pragma GCC reset_options
-
 #pragma GCC push_options
 #pragma GCC target("altivec,no-vsx")
 
@@ -33,6 +29,7 @@ 
 #error "__VSX__ should not be defined."
 #endif
 
+/* Altivec build, generate vaddfp.  */
 void
 av_add (vector float *a, vector float *b, vector float *c)
 {
@@ -40,10 +37,11 @@  av_add (vector float *a, vector float *b, vector float *c)
   unsigned long n = SIZE / 4;
 
   for (i = 0; i < n; i++)
-    a[i] = vec_add (b[i], c[i]);
+    a[i] = b[i] + c[i];
 }
 
-#pragma GCC target("vsx")
+/* cpu=power7 must be used to enable VSX.  */
+#pragma GCC target("cpu=power7,vsx")
 
 #ifndef __ALTIVEC__
 #error "__ALTIVEC__ should be defined."
@@ -53,6 +51,7 @@  av_add (vector float *a, vector float *b, vector float *c)
 #error "__VSX__ should be defined."
 #endif
 
+/* VSX build on power7, generate xsaddsp.  */
 void
 vsx_add (vector float *a, vector float *b, vector float *c)
 {
@@ -60,11 +59,31 @@  vsx_add (vector float *a, vector float *b, vector float *c)
   unsigned long n = SIZE / 4;
 
   for (i = 0; i < n; i++)
-    a[i] = vec_add (b[i], c[i]);
+    a[i] = b[i] + c[i];
+}
+
+#pragma GCC target("cpu=power7,no-vsx")
+
+#ifndef __ALTIVEC__
+#error "__ALTIVEC__ should be defined."
+#endif
+
+#ifdef __VSX__
+#error "__VSX__ should not be defined."
+#endif
+
+/* Altivec build on power7 with no VSX, generate vaddfp.  */
+void
+av2_add (vector float *a, vector float *b, vector float *c)
+{
+  unsigned long i;
+  unsigned long n = SIZE / 4;
+
+  for (i = 0; i < n; i++)
+    a[i] = b[i] + c[i];
 }
 
 #pragma GCC pop_options
-#pragma GCC target("no-vsx,no-altivec")
 
 #ifdef __ALTIVEC__
 #error "__ALTIVEC__ should not be defined."
@@ -74,6 +93,7 @@  vsx_add (vector float *a, vector float *b, vector float *c)
 #error "__VSX__ should not be defined."
 #endif
 
+/* Default power5 build, generate scalar fadds.  */
 void
 norm_add (float *a, float *b, float *c)
 {
diff --git a/gcc/testsuite/gcc.target/powerpc/pr115688.c b/gcc/testsuite/gcc.target/powerpc/pr115688.c
index 5222e66ef17..00c7c301436 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr115688.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr115688.c
@@ -7,7 +7,8 @@ 
 
 /* Verify there is no ICE under 32 bit env.  */
 
-__attribute__((target("vsx")))
+/* cpu=power7 must be used to enable VSX.  */
+__attribute__((target("cpu=power7,vsx")))
 int test (void)
 {
   return 0;