| Message ID | 20201115171747.GA10343@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | PowerPC Fix ibm128 defaults for pr70117.c test. | expand |
On Sun, 2020-11-15 at 12:17 -0500, Michael Meissner via Gcc-patches wrote: > From 698d9fd8a5701fa4ed9690ddf71d57765921778c Mon Sep 17 00:00:00 2001 > From: Michael Meissner <meissner@linux.ibm.com> > Date: Sun, 15 Nov 2020 00:48:23 -0500 > Subject: [PATCH] PowerPC Fix ibm128 defaults for pr70117.c test. > > This patch was previously posted as a combined patch with 2 other testsuite > patches. I moved it to a separate patch. I don't see that thread. Either really old or differently named ? > > This patch fixes up a failure that I saw when I built a compiler with the long > double default set to IEEE 128-bit instead of IBM 128-bit. Now compilers with > either 128-bit long double default pass this test. Can I check this into the > master branch? sandbox or upstream failure? Perhaps stands alone better as " This patch updates the pr70177.c testcase to define IBM128_MAX as appropriate for the IBM 128 or IEEE 128 type that is currently in use." ? > > gcc/testsuite/ > 2020-11-15 Michael Meissner <meissner@linux.ibm.com> > > PR target/70117 > * gcc.target/powerpc/pr70117.c: Add support for long double being > IEEE 128-bit. > --- > gcc/testsuite/gcc.target/powerpc/pr70117.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c b/gcc/testsuite/gcc.target/powerpc/pr70117.c > index 3bbd2c595e0..928efe39c7b 100644 > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > @@ -9,9 +9,11 @@ > 128-bit floating point, because the type is not enabled on those > systems. */ > #define LDOUBLE __ibm128 > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) > > #elif defined(__LONG_DOUBLE_IBM128__) > #define LDOUBLE long double > +#define IBM128_MAX LDBL_MAX > > #else > #error "long double must be either IBM 128-bit or IEEE 128-bit" > @@ -75,10 +77,10 @@ main (void) > if (__builtin_isnormal (ld)) > __builtin_abort (); > > - ld = LDBL_MAX; > + ld = IBM128_MAX; > if (!__builtin_isnormal (ld)) > __builtin_abort (); > - ld = -LDBL_MAX; > + ld = -IBM128_MAX; > if (!__builtin_isnormal (ld)) > __builtin_abort (); > > -- > 2.22.0 > >
Hi! On Sun, Nov 15, 2020 at 12:17:47PM -0500, Michael Meissner wrote: > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > @@ -9,9 +9,11 @@ > 128-bit floating point, because the type is not enabled on those > systems. */ > #define LDOUBLE __ibm128 > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) This is the IEEE QP float number 43fefffffffffffff7ffffffffffff80 which I very much doubt is the maximum finite double-double? See the 0 in the middle of the mantissa... 43feffffffffffffffffffffffffff00 is bigger, and representable as double-double just as well? Or even 43feffffffffffffffffffffffffff80 should be. Segher
On Wed, Nov 18, 2020 at 03:43:20PM -0600, Segher Boessenkool wrote: > Hi! > > On Sun, Nov 15, 2020 at 12:17:47PM -0500, Michael Meissner wrote: > > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > > @@ -9,9 +9,11 @@ > > 128-bit floating point, because the type is not enabled on those > > systems. */ > > #define LDOUBLE __ibm128 > > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) > > This is the IEEE QP float number 43fefffffffffffff7ffffffffffff80 which > I very much doubt is the maximum finite double-double? See the 0 in the Numbers without the 0 in the middle-end aren't valid, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95450#c6 for more details. Without the 0 in the middle the double double number rounded to double would require increasing the higher double, and as it is the largest representable finite number, that is not possible. Jakub
On Wed, Nov 18, 2020 at 10:53:49PM +0100, Jakub Jelinek wrote: > On Wed, Nov 18, 2020 at 03:43:20PM -0600, Segher Boessenkool wrote: > > Hi! > > > > On Sun, Nov 15, 2020 at 12:17:47PM -0500, Michael Meissner wrote: > > > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > @@ -9,9 +9,11 @@ > > > 128-bit floating point, because the type is not enabled on those > > > systems. */ > > > #define LDOUBLE __ibm128 > > > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) > > > > This is the IEEE QP float number 43fefffffffffffff7ffffffffffff80 which > > I very much doubt is the maximum finite double-double? See the 0 in the > > Numbers without the 0 in the middle-end aren't valid, see > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95450#c6 > for more details. Without the 0 in the middle the double double number > rounded to double would require increasing the higher double, and as it is > the largest representable finite number, that is not possible. Ah, in that way. Tricky. Mike, please add a comment, what number it represents? Okay for trunk with that, thanks. (Should those not be define in some header though?) Segher
On Wed, Nov 18, 2020 at 04:29:09PM -0600, Segher Boessenkool wrote: > On Wed, Nov 18, 2020 at 10:53:49PM +0100, Jakub Jelinek wrote: > > On Wed, Nov 18, 2020 at 03:43:20PM -0600, Segher Boessenkool wrote: > > > On Sun, Nov 15, 2020 at 12:17:47PM -0500, Michael Meissner wrote: > > > > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > > @@ -9,9 +9,11 @@ > > > > 128-bit floating point, because the type is not enabled on those > > > > systems. */ > > > > #define LDOUBLE __ibm128 > > > > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) > > > > > > This is the IEEE QP float number 43fefffffffffffff7ffffffffffff80 which > > > I very much doubt is the maximum finite double-double? See the 0 in the > > > > Numbers without the 0 in the middle-end aren't valid, see > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95450#c6 > > for more details. Without the 0 in the middle the double double number > > rounded to double would require increasing the higher double, and as it is > > the largest representable finite number, that is not possible. > > Ah, in that way. Tricky. > > Mike, please add a comment, what number it represents? Okay for trunk > with that, thanks. > > (Should those not be define in some header though?) Would it be better to represent the number in hex, like with printf's '%a' formatting (e.g. "0x1.921fb54442d18p+0"...this is NOT the same value)? (I always get nervous when I see a long float hardcoded in decimal.) PC
On Wed, Nov 18, 2020 at 04:29:09PM -0600, Segher Boessenkool wrote: > On Wed, Nov 18, 2020 at 10:53:49PM +0100, Jakub Jelinek wrote: > > On Wed, Nov 18, 2020 at 03:43:20PM -0600, Segher Boessenkool wrote: > > > Hi! > > > > > > On Sun, Nov 15, 2020 at 12:17:47PM -0500, Michael Meissner wrote: > > > > --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > > +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c > > > > @@ -9,9 +9,11 @@ > > > > 128-bit floating point, because the type is not enabled on those > > > > systems. */ > > > > #define LDOUBLE __ibm128 > > > > +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) > > > > > > This is the IEEE QP float number 43fefffffffffffff7ffffffffffff80 which > > > I very much doubt is the maximum finite double-double? See the 0 in the > > > > Numbers without the 0 in the middle-end aren't valid, see > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95450#c6 > > for more details. Without the 0 in the middle the double double number > > rounded to double would require increasing the higher double, and as it is > > the largest representable finite number, that is not possible. > > Ah, in that way. Tricky. > > Mike, please add a comment, what number it represents? Okay for trunk > with that, thanks. > > (Should those not be define in some header though?) When long double is IBM extended double, then LDBL_MAX, etc. is set with math.h (and the __ version created by the compiler). We don't have min/max for the funky MD only floating point numbers defined. I got the number by printing LDBL_MAX in fact and just pasting that in.
On Thu, Nov 19, 2020 at 03:08:05AM -0500, Michael Meissner wrote: > On Wed, Nov 18, 2020 at 04:29:09PM -0600, Segher Boessenkool wrote: > > Mike, please add a comment, what number it represents? Okay for trunk > > with that, thanks. > > > > (Should those not be define in some header though?) > > When long double is IBM extended double, then LDBL_MAX, etc. is set with math.h > (and the __ version created by the compiler). We don't have min/max for the > funky MD only floating point numbers defined. I got the number by printing > LDBL_MAX in fact and just pasting that in. Sure -- I am suggesting to always define __IBM128_MAX__ and the like, which then can be used to define LDBL_MAX, but also can be used directly. Segher
On Wed, Nov 18, 2020 at 07:13:04PM -0600, Paul A. Clarke wrote: > On Wed, Nov 18, 2020 at 04:29:09PM -0600, Segher Boessenkool wrote: > > Mike, please add a comment, what number it represents? Okay for trunk > > with that, thanks. > > > > (Should those not be define in some header though?) > > Would it be better to represent the number in hex, like with printf's '%a' > formatting (e.g. "0x1.921fb54442d18p+0"...this is NOT the same value)? > > (I always get nervous when I see a long float hardcoded in decimal.) Yeah, good point, thanks. That's indeed why I converted it to hex, to see what it *is* :-) Hexadecimal float is C99 so we cannot use it in the installed headers, but it should be fine in most testcases now. And even in the headers we can put it in a comment of course. Segher
On Thu, Nov 19, 2020 at 08:03:02AM -0600, Segher Boessenkool wrote: > On Thu, Nov 19, 2020 at 03:08:05AM -0500, Michael Meissner wrote: > > On Wed, Nov 18, 2020 at 04:29:09PM -0600, Segher Boessenkool wrote: > > > Mike, please add a comment, what number it represents? Okay for trunk > > > with that, thanks. > > > > > > (Should those not be define in some header though?) > > > > When long double is IBM extended double, then LDBL_MAX, etc. is set with math.h > > (and the __ version created by the compiler). We don't have min/max for the > > funky MD only floating point numbers defined. I got the number by printing > > LDBL_MAX in fact and just pasting that in. > > Sure -- I am suggesting to always define __IBM128_MAX__ and the like, > which then can be used to define LDBL_MAX, but also can be used > directly. I have posted patches for this as a new set of patches. Rather than trying to create IBM 128-bit long double min/max/etc. defines, I just marked the test as needing IBM 128-bit long double. I did look into providing defines for these. Unfortunately the function that creates these (builtin_define_float_constants) is static. And the caller of that function (c_cpp_builtins) does not have a target hook or other method to provide for these defines for MD specific floating point types.
On Sat, Nov 21, 2020 at 12:27:30AM -0500, Michael Meissner wrote: > On Thu, Nov 19, 2020 at 08:03:02AM -0600, Segher Boessenkool wrote: > > Sure -- I am suggesting to always define __IBM128_MAX__ and the like, > > which then can be used to define LDBL_MAX, but also can be used > > directly. > > I have posted patches for this as a new set of patches. Rather than trying to > create IBM 128-bit long double min/max/etc. defines, I just marked the test as > needing IBM 128-bit long double. > > I did look into providing defines for these. Unfortunately the function that > creates these (builtin_define_float_constants) is static. And the caller of > that function (c_cpp_builtins) does not have a target hook or other method to > provide for these defines for MD specific floating point types. So create the defines from somewhere in the backend, instead? This isn't rocket science. You can even leave the existing LDBL defines alone if you just override them later. And there is nothing against adding new hooks or whatever! Segher
On Mon, Nov 23, 2020 at 12:59:29PM -0600, Segher Boessenkool wrote: > On Sat, Nov 21, 2020 at 12:27:30AM -0500, Michael Meissner wrote: > > On Thu, Nov 19, 2020 at 08:03:02AM -0600, Segher Boessenkool wrote: > > > Sure -- I am suggesting to always define __IBM128_MAX__ and the like, > > > which then can be used to define LDBL_MAX, but also can be used > > > directly. > > > > I have posted patches for this as a new set of patches. Rather than trying to > > create IBM 128-bit long double min/max/etc. defines, I just marked the test as > > needing IBM 128-bit long double. > > > > I did look into providing defines for these. Unfortunately the function that > > creates these (builtin_define_float_constants) is static. And the caller of > > that function (c_cpp_builtins) does not have a target hook or other method to > > provide for these defines for MD specific floating point types. > > So create the defines from somewhere in the backend, instead? This > isn't rocket science. > > You can even leave the existing LDBL defines alone if you just override > them later. The current defines are quite complex code, because defining them is very expensive and we don't want to pay that price at every compilation start when most of the compilations never use those macros. So they are magic deferred macros. Jakub
On Mon, Nov 23, 2020 at 08:10:49PM +0100, Jakub Jelinek wrote: > On Mon, Nov 23, 2020 at 12:59:29PM -0600, Segher Boessenkool wrote: > > On Sat, Nov 21, 2020 at 12:27:30AM -0500, Michael Meissner wrote: > > > On Thu, Nov 19, 2020 at 08:03:02AM -0600, Segher Boessenkool wrote: > > > > Sure -- I am suggesting to always define __IBM128_MAX__ and the like, > > > > which then can be used to define LDBL_MAX, but also can be used > > > > directly. > > > > > > I have posted patches for this as a new set of patches. Rather than trying to > > > create IBM 128-bit long double min/max/etc. defines, I just marked the test as > > > needing IBM 128-bit long double. > > > > > > I did look into providing defines for these. Unfortunately the function that > > > creates these (builtin_define_float_constants) is static. And the caller of > > > that function (c_cpp_builtins) does not have a target hook or other method to > > > provide for these defines for MD specific floating point types. > > > > So create the defines from somewhere in the backend, instead? This > > isn't rocket science. > > > > You can even leave the existing LDBL defines alone if you just override > > them later. > > The current defines are quite complex code, because defining them is very > expensive and we don't want to pay that price at every compilation start > when most of the compilations never use those macros. > So they are magic deferred macros. Ah, so defining later will not work, or it will be quite fragile at least. So that won't fly :-/ We can provide IBM128 etc. macros for this just fine of course, but using that for the LDBL is hard then, so perhaps we should not do that second step, certainly not in stage > 1. Thanks, Segher
diff --git a/gcc/testsuite/gcc.target/powerpc/pr70117.c b/gcc/testsuite/gcc.target/powerpc/pr70117.c index 3bbd2c595e0..928efe39c7b 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr70117.c +++ b/gcc/testsuite/gcc.target/powerpc/pr70117.c @@ -9,9 +9,11 @@ 128-bit floating point, because the type is not enabled on those systems. */ #define LDOUBLE __ibm128 +#define IBM128_MAX ((__ibm128) 1.79769313486231580793728971405301199e+308L) #elif defined(__LONG_DOUBLE_IBM128__) #define LDOUBLE long double +#define IBM128_MAX LDBL_MAX #else #error "long double must be either IBM 128-bit or IEEE 128-bit" @@ -75,10 +77,10 @@ main (void) if (__builtin_isnormal (ld)) __builtin_abort (); - ld = LDBL_MAX; + ld = IBM128_MAX; if (!__builtin_isnormal (ld)) __builtin_abort (); - ld = -LDBL_MAX; + ld = -IBM128_MAX; if (!__builtin_isnormal (ld)) __builtin_abort ();
From 698d9fd8a5701fa4ed9690ddf71d57765921778c Mon Sep 17 00:00:00 2001 From: Michael Meissner <meissner@linux.ibm.com> Date: Sun, 15 Nov 2020 00:48:23 -0500 Subject: [PATCH] PowerPC Fix ibm128 defaults for pr70117.c test. This patch was previously posted as a combined patch with 2 other testsuite patches. I moved it to a separate patch. This patch fixes up a failure that I saw when I built a compiler with the long double default set to IEEE 128-bit instead of IBM 128-bit. Now compilers with either 128-bit long double default pass this test. Can I check this into the master branch? gcc/testsuite/ 2020-11-15 Michael Meissner <meissner@linux.ibm.com> PR target/70117 * gcc.target/powerpc/pr70117.c: Add support for long double being IEEE 128-bit. --- gcc/testsuite/gcc.target/powerpc/pr70117.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)