diff mbox series

, PR target/99708 - Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__

Message ID YgvntWSjngfiP75i@toto.the-meissners.org
State New
Headers
Series , PR target/99708 - Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ |

Commit Message

Michael Meissner Feb. 15, 2022, 5:49 p.m. UTC 
  Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__.

Define the sizes of the PowerPC specific types __float128 and __ibm128 if those
types are enabled.

I tested this on a little endian power9 system and there were no regressions.
Can I check this into the trunk, and after a burn-in period, can I check this
into the GCC 10 and 11 branches?

2022-02-15  Michael Meissner  <meissner@the-meissners.org>

gcc/
	PR target/99708
	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
	__SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ if we have float128
	support.

gcc/testsuite/
	PR target/99708
	* gcc.target/powerpc/pr99708.c: New test.
---
 gcc/config/rs6000/rs6000-c.cc              | 12 ++++++++++--
 gcc/testsuite/gcc.target/powerpc/pr99708.c | 21 +++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr99708.c

Comments

Segher Boessenkool Feb. 15, 2022, 7:45 p.m. UTC | #1 
On Tue, Feb 15, 2022 at 12:49:41PM -0500, Michael Meissner wrote:
> Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__.
> 
> Define the sizes of the PowerPC specific types __float128 and __ibm128 if those
> types are enabled.

This is very silly of course, both of these are 16 bytes.  Abusing this
to see if the types exist is at least as silly (there are much better
mechanisms to do this).

So, this facilitates bad habits and bad code.  But, whatever, the macros
are just stating totally obvious and redundant facts, no problem, let's
just ignore that pepople only want it to abuse it.

> gcc/
> 	PR target/99708
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> 	__SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ if we have float128
> 	support.

No.  __SIZEOF_IBM128__ should be defined if and only if __ibm128 is
defined.

This should be tested directly, it should not depend on that some other
code did what it does today.  That would also make the code much more
obvious.


Segher
Michael Meissner Feb. 15, 2022, 8:18 p.m. UTC | #2 
On Tue, Feb 15, 2022 at 01:45:06PM -0600, Segher Boessenkool wrote:
> On Tue, Feb 15, 2022 at 12:49:41PM -0500, Michael Meissner wrote:
> > Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__.
> > 
> > Define the sizes of the PowerPC specific types __float128 and __ibm128 if those
> > types are enabled.
> 
> This is very silly of course, both of these are 16 bytes.  Abusing this
> to see if the types exist is at least as silly (there are much better
> mechanisms to do this).

This is just trying to close out the PR that people asked for.

> So, this facilitates bad habits and bad code.  But, whatever, the macros
> are just stating totally obvious and redundant facts, no problem, let's
> just ignore that pepople only want it to abuse it.
> 
> > gcc/
> > 	PR target/99708
> > 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> > 	__SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ if we have float128
> > 	support.
> 
> No.  __SIZEOF_IBM128__ should be defined if and only if __ibm128 is
> defined.

In the current implementation, __ibm128 is only defined if __float128 was
defined.  I did have patches 6 months or so to define __ibm128 on any system
with IBM 128-bit long double.  But those were never applied to the trunk.

> This should be tested directly, it should not depend on that some other
> code did what it does today.  That would also make the code much more
> obvious.

Given __float128 and __ibm128 are defined at the same time, I don't see the
need for a separate target-supports.exp test, and so forth.  But if you want
that, I can easily do it.
Segher Boessenkool Feb. 15, 2022, 10:05 p.m. UTC | #3 
On Tue, Feb 15, 2022 at 03:18:30PM -0500, Michael Meissner wrote:
> On Tue, Feb 15, 2022 at 01:45:06PM -0600, Segher Boessenkool wrote:
> > On Tue, Feb 15, 2022 at 12:49:41PM -0500, Michael Meissner wrote:
> > > Define __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__.
> > > 
> > > Define the sizes of the PowerPC specific types __float128 and __ibm128 if those
> > > types are enabled.
> > 
> > This is very silly of course, both of these are 16 bytes.  Abusing this
> > to see if the types exist is at least as silly (there are much better
> > mechanisms to do this).
> 
> This is just trying to close out the PR that people asked for.

I put many arguments in that PR why this is a futile thing to have.

On all older compilers these macros will not be defined, but the types
often are.  If you are willing to not support older compilers properly
anyway, you could just *always* use the types, which will work with most
very old compilers as well (and the approach using these propesed
predefines will *not*!)

This is (not) solving a non-problem.

> > So, this facilitates bad habits and bad code.  But, whatever, the macros
> > are just stating totally obvious and redundant facts, no problem, let's
> > just ignore that pepople only want it to abuse it.
> > 
> > > gcc/
> > > 	PR target/99708
> > > 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> > > 	__SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ if we have float128
> > > 	support.
> > 
> > No.  __SIZEOF_IBM128__ should be defined if and only if __ibm128 is
> > defined.
> 
> In the current implementation, __ibm128 is only defined if __float128 was
> defined.  I did have patches 6 months or so to define __ibm128 on any system
> with IBM 128-bit long double.  But those were never applied to the trunk.
> 
> > This should be tested directly, it should not depend on that some other
> > code did what it does today.  That would also make the code much more
> > obvious.

Read that last paragraph again please?  This is the only thing that
needs changing.

> Given __float128 and __ibm128 are defined at the same time, I don't see the
> need for a separate target-supports.exp test, and so forth.  But if you want
> that, I can easily do it.

No, I'm asking for it in the code that does the predefine.  Not in the
testcase, testcases are dirty pretty much always, and it is acceptable
there because the scope is so limited.


Segher
Michael Meissner Feb. 15, 2022, 11:05 p.m. UTC | #4 
On Tue, Feb 15, 2022 at 04:05:11PM -0600, Segher Boessenkool wrote:
> On all older compilers these macros will not be defined, but the types
> often are.  If you are willing to not support older compilers properly
> anyway, you could just *always* use the types, which will work with most
> very old compilers as well (and the approach using these propesed
> predefines will *not*!)

The types are not defined on older systems.

Both __ibm128 (ibm128_float_type_node) and __float128 (ieee128_float_type_node)
are only defined if TARGET_FLOAT128_TYPE is true.

TARGET_FLOAT128_TYPE is only true if both TARGET_FLOAT128_ENABLE_TYPE and
TARGET_VSX are true.

TARGET_FLOAT128_ENABLE_TYPE is only true on linux64 systems.

Now, the code to set __SIZEOF_IBM128__ and __SIZEOF_FLOAT128__  is in the code
that also defines __FLOAT128__.  This code checks whether the __float128 and
__ibm128 keywords are allowed.  These keywords are only set if
TARGET_FLOAT128_TYPE is true, and if the user did not use the -mno-float128
option.  In the GCC 7 time frame, we did not set this by default, but in the
modern compilers, it is always set by default on Linux 64-bit systems.
Segher Boessenkool Feb. 16, 2022, 10:28 a.m. UTC | #5 
On Tue, Feb 15, 2022 at 06:05:06PM -0500, Michael Meissner wrote:
> On Tue, Feb 15, 2022 at 04:05:11PM -0600, Segher Boessenkool wrote:
> > On all older compilers these macros will not be defined, but the types
> > often are.  If you are willing to not support older compilers properly
> > anyway, you could just *always* use the types, which will work with most
> > very old compilers as well (and the approach using these propesed
> > predefines will *not*!)
> 
> The types are not defined on older systems.

They are defined since GCC 6.  Using new macros to see if the types
exist will miss all of GCC 6, 7, 8, 9, 10, 11.

> Both __ibm128 (ibm128_float_type_node) and __float128 (ieee128_float_type_node)
> are only defined if TARGET_FLOAT128_TYPE is true.
> 
> TARGET_FLOAT128_TYPE is only true if both TARGET_FLOAT128_ENABLE_TYPE and
> TARGET_VSX are true.
> 
> TARGET_FLOAT128_ENABLE_TYPE is only true on linux64 systems.
> 
> Now, the code to set __SIZEOF_IBM128__ and __SIZEOF_FLOAT128__  is in the code
> that also defines __FLOAT128__.  This code checks whether the __float128 and
> __ibm128 keywords are allowed.  These keywords are only set if
> TARGET_FLOAT128_TYPE is true, and if the user did not use the -mno-float128
> option.  In the GCC 7 time frame, we did not set this by default, but in the
> modern compilers, it is always set by default on Linux 64-bit systems.

__SIZEOF_IBM128__ should be defined based on what we have for the
__ibm128 type, not on what we have for the __float128 type  Yes I know
we *currently* define those under the same conditions, but why write
code that is more fragile than needed?  Please don't.  It is easy to do
it correctly, so it is no real hassle for the writer of the code, and it
is much better for the reader.


Segher
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index 15251efc209..b5f771d1308 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -581,9 +581,17 @@  rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
     {
       rs6000_define_or_undefine_macro (define_p, "__FLOAT128__");
       if (define_p)
-	rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
+	{
+	  rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
+	  rs6000_define_or_undefine_macro (true, "__SIZEOF_FLOAT128__=16");
+	  rs6000_define_or_undefine_macro (true, "__SIZEOF_IBM128__=16");
+	}
       else
-	rs6000_define_or_undefine_macro (false, "__float128");
+	{
+	  rs6000_define_or_undefine_macro (false, "__float128");
+	  rs6000_define_or_undefine_macro (false, "__SIZEOF_FLOAT128__");
+	  rs6000_define_or_undefine_macro (false, "__SIZEOF_IBM128__");
+	}
     }
   /* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
      via the target attribute/pragma.  */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr99708.c b/gcc/testsuite/gcc.target/powerpc/pr99708.c
new file mode 100644
index 00000000000..d478f7bc4c0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr99708.c
@@ -0,0 +1,21 @@ 
+/* { dg-do run } */
+/* { require-effective-target ppc_float128_sw } */
+/* { dg-options "-O2 -mvsx -mfloat128" } */
+
+/*
+ * PR target/99708
+ *
+ * Verify that __SIZEOF_FLOAT128__ and __SIZEOF_IBM128__ are properly defined.
+ */
+
+#include <stdlib.h>
+
+int main (void)
+{
+  if (__SIZEOF_FLOAT128__ != sizeof (__float128)
+      || __SIZEOF_IBM128__ != sizeof (__ibm128))
+    abort ();
+
+  return 0;
+}
+