[2/3] Fix IEEE 128-bit min/max test.

Message ID 20210609002240.GB18854@ibm-toto.the-meissners.org
State New
Headers show
Series
  • Add Power10 IEEE 128-bit min, max, conditional move
Related show

Commit Message

Jason Merrill via Gcc-patches June 9, 2021, 12:22 a.m.
[PATCH 2/3] Fix IEEE 128-bit min/max test.

This patch fixes the float128-minmax.c test so that it can accommodate the
generation of xsmincqp and xsmaxcqp instructions on power10.  I changed
the effective target from 'float128' to 'ppc_float128_hw', since this
needs the IEEE 128-bit float hardware support.

I tested it on 3 platforms:

    *	Power9 little endian, --with-code=power9;
    *	Power8 big endian, --with-code=power8, both 32/64-bit tests done;
    *	Power10 little endian, --with-code=power10.

All systems bootstrapped and there were no new regressions.  I believe I have
addressed the issues with the last patch.

Can I check this into the master branch, and after a soak-in period, back port
it to the GCC 11 branch?

gcc/testsuite/
2021-06-08  Michael Meissner  <meissner@linux.ibm.com>

	* gcc.target/powerpc/float128-minmax.c: Adjust expected code for
	power10.
	* lib/target-supports.exp (check_effective_target_has_arch_pwr10):
	New target support.
---
 gcc/testsuite/gcc.target/powerpc/float128-minmax.c |  8 +++++---
 gcc/testsuite/lib/target-supports.exp              | 10 ++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.31.1


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Comments

Jason Merrill via Gcc-patches June 17, 2021, 12:33 a.m. | #1
Ping patch.  In particular, we like to get this into GCC 11.2 as part of
power10 enablement.

| Date: Tue, 8 Jun 2021 20:22:40 -0400
| Subject: [PATCH 2/3] Fix IEEE 128-bit min/max test.
| Message-ID: <20210609002240.GB18854@ibm-toto.the-meissners.org>

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Segher Boessenkool June 17, 2021, 6:11 p.m. | #2
On Tue, Jun 08, 2021 at 08:22:40PM -0400, Michael Meissner wrote:
> 

> 	* gcc.target/powerpc/float128-minmax.c: Adjust expected code for

> 	power10.

> 	* lib/target-supports.exp (check_effective_target_has_arch_pwr10):

> 	New target support.

> ---

>  gcc/testsuite/gcc.target/powerpc/float128-minmax.c |  8 +++++---

>  gcc/testsuite/lib/target-supports.exp              | 10 ++++++++++

>  2 files changed, 15 insertions(+), 3 deletions(-)

> 

> diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> index fe397518f2f..a7d3a3a0b3e 100644

> --- a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> @@ -1,6 +1,5 @@

> -/* { dg-do compile { target lp64 } } */


Does that work?  Why was it there before?

>  /* { dg-require-effective-target powerpc_p9vector_ok } */

> -/* { dg-require-effective-target float128 } */

> +/* { dg-require-effective-target ppc_float128_hw } */


Why is it okay to no longer run this test where it ran before?

> -/* { dg-final { scan-assembler     {\mxscmpuqp\M} } } */

> +/* Adjust code power10 which has native min/max instructions.  */

> +/* { dg-final { scan-assembler     {\mxscmpuqp\M} { target { ! has_arch_pwr10 } } } } */


You need scan-assembler-times here?  (Not that it had that before this
patch, of course).

> +/* { dg-final { scan-assembler     {\mxsmincqp\M} { target {   has_arch_pwr10 } } } } */

> +/* { dg-final { scan-assembler     {\mxsmaxcqp\M} { target {   has_arch_pwr10 } } } } */


You can write just  { target has_arch_pwr10 }  here, I think?  Please do
so (if that works, I haven't actually tested it :-) )


Segher
Jason Merrill via Gcc-patches June 17, 2021, 7:24 p.m. | #3
On Thu, Jun 17, 2021 at 01:11:58PM -0500, Segher Boessenkool wrote:
> On Tue, Jun 08, 2021 at 08:22:40PM -0400, Michael Meissner wrote:

> > 

> > 	* gcc.target/powerpc/float128-minmax.c: Adjust expected code for

> > 	power10.

> > 	* lib/target-supports.exp (check_effective_target_has_arch_pwr10):

> > 	New target support.

> > ---

> >  gcc/testsuite/gcc.target/powerpc/float128-minmax.c |  8 +++++---

> >  gcc/testsuite/lib/target-supports.exp              | 10 ++++++++++

> >  2 files changed, 15 insertions(+), 3 deletions(-)

> > 

> > diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > index fe397518f2f..a7d3a3a0b3e 100644

> > --- a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > @@ -1,6 +1,5 @@

> > -/* { dg-do compile { target lp64 } } */

> 

> Does that work?  Why was it there before?

> 

> >  /* { dg-require-effective-target powerpc_p9vector_ok } */

> > -/* { dg-require-effective-target float128 } */

> > +/* { dg-require-effective-target ppc_float128_hw } */

> 

> Why is it okay to no longer run this test where it ran before?


I was just being more precise.

> > -/* { dg-final { scan-assembler     {\mxscmpuqp\M} } } */

> > +/* Adjust code power10 which has native min/max instructions.  */

> > +/* { dg-final { scan-assembler     {\mxscmpuqp\M} { target { ! has_arch_pwr10 } } } } */

> 

> You need scan-assembler-times here?  (Not that it had that before this

> patch, of course).


I can change it to scan-assembler-times.

> > +/* { dg-final { scan-assembler     {\mxsmincqp\M} { target {   has_arch_pwr10 } } } } */

> > +/* { dg-final { scan-assembler     {\mxsmaxcqp\M} { target {   has_arch_pwr10 } } } } */

> 

> You can write just  { target has_arch_pwr10 }  here, I think?  Please do

> so (if that works, I haven't actually tested it :-) )


I thought so originally, but it didn't like it, so I added the extra {}'s.  But
I can try it again.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Jason Merrill via Gcc-patches June 17, 2021, 8:11 p.m. | #4
On Thu, Jun 17, 2021 at 01:11:58PM -0500, Segher Boessenkool wrote:
> On Tue, Jun 08, 2021 at 08:22:40PM -0400, Michael Meissner wrote:

> > 

> > 	* gcc.target/powerpc/float128-minmax.c: Adjust expected code for

> > 	power10.

> > 	* lib/target-supports.exp (check_effective_target_has_arch_pwr10):

> > 	New target support.

> > ---

> >  gcc/testsuite/gcc.target/powerpc/float128-minmax.c |  8 +++++---

> >  gcc/testsuite/lib/target-supports.exp              | 10 ++++++++++

> >  2 files changed, 15 insertions(+), 3 deletions(-)

> > 

> > diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > index fe397518f2f..a7d3a3a0b3e 100644

> > --- a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c

> > @@ -1,6 +1,5 @@

> > -/* { dg-do compile { target lp64 } } */

> 

> Does that work?  Why was it there before?


The lp64 eliminates 32-bit, which does not support hardware IEEE 128-bit due to
the lack of TImode.  The test was written before the ppc_float128_hw test.  Now
that we have ppc_float128_hw, we don't need an explicit lp64.
 
> >  /* { dg-require-effective-target powerpc_p9vector_ok } */

> > -/* { dg-require-effective-target float128 } */

> > +/* { dg-require-effective-target ppc_float128_hw } */

> 

> Why is it okay to no longer run this test where it ran before?


The ppc_float128_hw test is a more precise test than just float128 and power9.
For 64-bit, it doesn't matter, but it allows us to drop the lp64 test.
 
> > -/* { dg-final { scan-assembler     {\mxscmpuqp\M} } } */

> > +/* Adjust code power10 which has native min/max instructions.  */

> > +/* { dg-final { scan-assembler     {\mxscmpuqp\M} { target { ! has_arch_pwr10 } } } } */

> 

> You need scan-assembler-times here?  (Not that it had that before this

> patch, of course).

> 

> > +/* { dg-final { scan-assembler     {\mxsmincqp\M} { target {   has_arch_pwr10 } } } } */

> > +/* { dg-final { scan-assembler     {\mxsmaxcqp\M} { target {   has_arch_pwr10 } } } } */

> 

> You can write just  { target has_arch_pwr10 }  here, I think?  Please do

> so (if that works, I haven't actually tested it :-) )

> 

> 

> Segher


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Patch

diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
index fe397518f2f..a7d3a3a0b3e 100644
--- a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
+++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
@@ -1,6 +1,5 @@ 
-/* { dg-do compile { target lp64 } } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-require-effective-target float128 } */
+/* { dg-require-effective-target ppc_float128_hw } */
 /* { dg-options "-mpower9-vector -O2 -ffast-math" } */
 
 #ifndef TYPE
@@ -12,5 +11,8 @@ 
 TYPE f128_min (TYPE a, TYPE b) { return __builtin_fminf128 (a, b); }
 TYPE f128_max (TYPE a, TYPE b) { return __builtin_fmaxf128 (a, b); }
 
-/* { dg-final { scan-assembler     {\mxscmpuqp\M} } } */
+/* Adjust code power10 which has native min/max instructions.  */
+/* { dg-final { scan-assembler     {\mxscmpuqp\M} { target { ! has_arch_pwr10 } } } } */
+/* { dg-final { scan-assembler     {\mxsmincqp\M} { target {   has_arch_pwr10 } } } } */
+/* { dg-final { scan-assembler     {\mxsmaxcqp\M} { target {   has_arch_pwr10 } } } } */
 /* { dg-final { scan-assembler-not {\mbl\M}       } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 7f78c5593ac..789723fb287 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -6127,6 +6127,16 @@  proc check_effective_target_has_arch_pwr9 { } {
 	}]
 }
 
+proc check_effective_target_has_arch_pwr10 { } {
+	return [check_no_compiler_messages arch_pwr10 assembly {
+		#ifndef _ARCH_PWR10
+		#error does not have power10 support.
+		#else
+		/* "has power10 support" */
+		#endif
+	}]
+}
+
 # Return 1 if this is a PowerPC target supporting -mcpu=power10.
 # Limit this to 64-bit linux systems for now until other targets support
 # power10.