[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

Jeff Law 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

Jeff Law 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
Jeff Law 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
Jeff Law 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
Segher Boessenkool June 25, 2021, 5:46 p.m. | #5
On Thu, Jun 17, 2021 at 04:11:40PM -0400, Michael Meissner wrote:
> On Thu, Jun 17, 2021 at 01:11:58PM -0500, Segher Boessenkool wrote:

> > > --- 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.


I still do not understand this.  Why would support for QP float require
TImode?  "Need an integer mode of the same size" is not a convincing
argument, since double-double is a 16 byte mode as well.

> The test was written before the ppc_float128_hw test.  Now

> that we have ppc_float128_hw, we don't need an explicit lp64.


Ah good, some progress.  Well, it *is* an improvement, a better
abstraction, but on the other hand it only hides the actual problems
deeper :-/

> > >  /* { 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.


You did not delete the p9 test though.


Segher
Jeff Law via Gcc-patches June 28, 2021, 6:41 p.m. | #6
On Fri, Jun 25, 2021 at 12:46:37PM -0500, Segher Boessenkool wrote:
> On Thu, Jun 17, 2021 at 04:11:40PM -0400, Michael Meissner wrote:

> > On Thu, Jun 17, 2021 at 01:11:58PM -0500, Segher Boessenkool wrote:

> > > > --- 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.

> 

> I still do not understand this.  Why would support for QP float require

> TImode?  "Need an integer mode of the same size" is not a convincing

> argument, since double-double is a 16 byte mode as well.


I suspect it is because we separate moves for IBM long double before the pass
that wants to use an integer type to do the move, so it doesn't see the 128-bit
type.

> > The test was written before the ppc_float128_hw test.  Now

> > that we have ppc_float128_hw, we don't need an explicit lp64.

> 

> Ah good, some progress.  Well, it *is* an improvement, a better

> abstraction, but on the other hand it only hides the actual problems

> deeper :-/

> 

> > > >  /* { 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.

> 

> You did not delete the p9 test though.


Yes, I can probably delete the powerpc_p9vector_ok test.

-- 
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.