* [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ defines [PR99708]
@ 2022-03-05 8:21 Jakub Jelinek
2022-03-07 21:37 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT, IBM}128__ " Segher Boessenkool
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2022-03-05 8:21 UTC (permalink / raw)
To: Segher Boessenkool, David Edelsohn
Cc: gcc-patches, Michael Meissner, Jonathan Wakely
Hi!
As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
actually supported or not.
The following patch:
1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
really support them instead of equal to long_double_type_node
2) adjusts the builtins code to use
ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
for the 2 builtins, so that we don't ICE during builtins creation
if ibm128_float_type_node is NULL (using error_mark_node instead of
long_double_type_node sounds more risky to me)
3) in rs6000_type_string will not match NULL as __ibm128, and adds
a __ieee128 case
4) removes the clearly unused ptr_{ieee,ibm}128_float_type_node trees;
if something needs them in the future, they can be easily added back,
but wasting GTY just in case...
5) actually syncs __SIZEOF_FLOAT128__ with the __float128 macro being
defined in addition to __ieee128 being usable
Now, in the PR Segher said he doesn't like 5), but I think it is better
to match the reality and get this PR fixed and if we want to rethink
how __float128 is defined (whether it is a macro, or perhaps another
builtin type like __ieee128 which could be easily done by
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__ieee128");
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__float128");
perhaps under some conditions, rethink if the -mfloat128 option exists
and what it does etc., it can be done incrementally and the rs6000-c.cc
hunks in the patch can be easily reverted (appart from the formatting
fix).
Bootstrapped/regtested on powerpc{64le,64}-linux, on powerpc64-linux
with -m32/-m64 testing, ok for trunk?
2022-03-05 Jakub Jelinek <jakub@redhat.com>
PR target/99708
* config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Remove
RS6000_BTI_ptr_ieee128_float and RS6000_BTI_ptr_ibm128_float.
(ptr_ieee128_float_type_node, ptr_ibm128_float_type_node): Remove.
* config/rs6000/rs6000-builtin.cc (rs6000_type_string): Return
"unknown" if type_node is NULL first. Handle ieee128_float_type_node.
(rs6000_init_builtins): Don't initialize ptr_ieee128_float_type_node
and ptr_ibm128_float_type_node. Set ibm128_float_type_node and
ieee128_float_type_node to NULL rather than long_double_type_node if
they aren't supported.
* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
__SIZEOF_FLOAT128__ here and only if __float128 macro is defined.
(rs6000_cpu_cpp_builtins): Don't define __SIZEOF_FLOAT128__ here.
Formatting fix.
* config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
85 instead of 86.
(type_map): For "if" use ibm128_float_type_node only if it is
non-NULL, otherwise fall back to long_double_type_node. Remove "pif"
entry.
* gcc.dg/pr99708.c: New test.
* gcc.target/powerpc/pr99708-2.c: New test.
--- gcc/config/rs6000/rs6000.h.jj 2022-02-04 14:36:54.546611710 +0100
+++ gcc/config/rs6000/rs6000.h 2022-03-04 16:45:59.962329746 +0100
@@ -2444,8 +2444,6 @@ enum rs6000_builtin_type_index
RS6000_BTI_ptr_long_double,
RS6000_BTI_ptr_dfloat64,
RS6000_BTI_ptr_dfloat128,
- RS6000_BTI_ptr_ieee128_float,
- RS6000_BTI_ptr_ibm128_float,
RS6000_BTI_ptr_vector_pair,
RS6000_BTI_ptr_vector_quad,
RS6000_BTI_ptr_long_long,
@@ -2541,8 +2539,6 @@ enum rs6000_builtin_type_index
#define ptr_long_double_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_double])
#define ptr_dfloat64_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat64])
#define ptr_dfloat128_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat128])
-#define ptr_ieee128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ieee128_float])
-#define ptr_ibm128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ibm128_float])
#define ptr_vector_pair_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_pair])
#define ptr_vector_quad_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_quad])
#define ptr_long_long_integer_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_long])
--- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-02-04 14:36:54.499612366 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-04 16:46:07.978218984 +0100
@@ -402,7 +402,9 @@ rs6000_vector_type (const char *name, tr
static
const char *rs6000_type_string (tree type_node)
{
- if (type_node == void_type_node)
+ if (type_node == NULL_TREE)
+ return "unknown";
+ else if (type_node == void_type_node)
return "void";
else if (type_node == long_integer_type_node)
return "long";
@@ -432,6 +434,8 @@ const char *rs6000_type_string (tree typ
return "ss";
else if (type_node == ibm128_float_type_node)
return "__ibm128";
+ else if (type_node == ieee128_float_type_node)
+ return "__ieee128";
else if (type_node == opaque_V4SI_type_node)
return "opaque";
else if (POINTER_TYPE_P (type_node))
@@ -721,7 +725,6 @@ rs6000_init_builtins (void)
layout_type (ibm128_float_type_node);
}
t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
- ptr_ibm128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ibm128_float_type_node,
"__ibm128");
@@ -730,13 +733,11 @@ rs6000_init_builtins (void)
else
ieee128_float_type_node = float128_type_node;
t = build_qualified_type (ieee128_float_type_node, TYPE_QUAL_CONST);
- ptr_ieee128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__ieee128");
}
-
else
- ieee128_float_type_node = ibm128_float_type_node = long_double_type_node;
+ ieee128_float_type_node = ibm128_float_type_node = NULL_TREE;
/* Vector pair and vector quad support. */
vector_pair_type_node = make_node (OPAQUE_TYPE);
--- gcc/config/rs6000/rs6000-c.cc.jj 2022-03-03 22:09:11.987942537 +0100
+++ gcc/config/rs6000/rs6000-c.cc 2022-03-04 16:22:50.616681130 +0100
@@ -584,6 +584,10 @@ rs6000_target_modify_macros (bool define
rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
else
rs6000_define_or_undefine_macro (false, "__float128");
+ if (ieee128_float_type_node && define_p)
+ rs6000_define_or_undefine_macro (true, "__SIZEOF_FLOAT128__=16");
+ else
+ rs6000_define_or_undefine_macro (false, "__SIZEOF_FLOAT128__");
}
/* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
via the target attribute/pragma. */
@@ -623,11 +627,9 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfi
if (TARGET_FRSQRTES)
builtin_define ("__RSQRTEF__");
if (TARGET_FLOAT128_TYPE)
- builtin_define ("__FLOAT128_TYPE__");
+ builtin_define ("__FLOAT128_TYPE__");
if (ibm128_float_type_node)
builtin_define ("__SIZEOF_IBM128__=16");
- if (ieee128_float_type_node)
- builtin_define ("__SIZEOF_FLOAT128__=16");
#ifdef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
builtin_define ("__BUILTIN_CPU_SUPPORTS__");
#endif
--- gcc/config/rs6000/rs6000-gen-builtins.cc.jj 2022-02-04 14:36:54.515612143 +0100
+++ gcc/config/rs6000/rs6000-gen-builtins.cc 2022-03-04 16:45:50.454461123 +0100
@@ -492,7 +492,7 @@ struct typemap
maps tokens from a fntype string to a tree type. For example,
in "si_ftype_hi" we would map "si" to "intSI_type_node" and
map "hi" to "intHI_type_node". */
-#define TYPE_MAP_SIZE 86
+#define TYPE_MAP_SIZE 85
static typemap type_map[TYPE_MAP_SIZE] =
{
{ "bi", "bool_int" },
@@ -506,7 +506,9 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "df", "double" },
{ "di", "long_long_integer" },
{ "hi", "intHI" },
- { "if", "ibm128_float" },
+ { "if", "ibm128_float_type_node "
+ "? ibm128_float_type_node "
+ ": long_double" },
{ "ld", "long_double" },
{ "lg", "long_integer" },
{ "pbv16qi", "ptr_bool_V16QI" },
@@ -519,7 +521,6 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "pdf", "ptr_double" },
{ "pdi", "ptr_long_long_integer" },
{ "phi", "ptr_intHI" },
- { "pif", "ptr_ibm128_float" },
{ "pld", "ptr_long_double" },
{ "plg", "ptr_long_integer" },
{ "pqi", "ptr_intQI" },
--- gcc/testsuite/gcc.dg/pr99708.c.jj 2022-03-04 18:34:21.603828608 +0100
+++ gcc/testsuite/gcc.dg/pr99708.c 2022-03-04 18:34:30.089709852 +0100
@@ -0,0 +1,7 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_FLOAT128__
+__float128 f = 1.0;
+#endif
+long double l = 1.0;
--- gcc/testsuite/gcc.target/powerpc/pr99708-2.c.jj 2022-03-04 18:35:29.393879918 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr99708-2.c 2022-03-04 18:35:43.545681867 +0100
@@ -0,0 +1,7 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_IBM128__
+__ibm128 f = 1.0;
+#endif
+long double l = 1.0;
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-05 8:21 [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ defines [PR99708] Jakub Jelinek
@ 2022-03-07 21:37 ` Segher Boessenkool
2022-03-09 13:27 ` [PATCH] rs6000, v2: " Jakub Jelinek
2022-03-10 20:44 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ " Michael Meissner
0 siblings, 2 replies; 13+ messages in thread
From: Segher Boessenkool @ 2022-03-07 21:37 UTC (permalink / raw)
To: Jakub Jelinek
Cc: David Edelsohn, gcc-patches, Michael Meissner, Jonathan Wakely
Hi!
On Sat, Mar 05, 2022 at 09:21:51AM +0100, Jakub Jelinek wrote:
> As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
> macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
> is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
> actually supported or not.
>
> The following patch:
> 1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
> really support them instead of equal to long_double_type_node
> 2) adjusts the builtins code to use
> ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> for the 2 builtins, so that we don't ICE during builtins creation
> if ibm128_float_type_node is NULL (using error_mark_node instead of
> long_double_type_node sounds more risky to me)
I feel the opposite way: (potentially) using the wrong thing is just a
ticking time bomb, never "safer".
> 3) in rs6000_type_string will not match NULL as __ibm128, and adds
> a __ieee128 case
> 4) removes the clearly unused ptr_{ieee,ibm}128_float_type_node trees;
> if something needs them in the future, they can be easily added back,
> but wasting GTY just in case...
> 5) actually syncs __SIZEOF_FLOAT128__ with the __float128 macro being
> defined in addition to __ieee128 being usable
>
> Now, in the PR Segher said he doesn't like 5), but I think it is better
> to match the reality and get this PR fixed and if we want to rethink
> how __float128 is defined (whether it is a macro, or perhaps another
> builtin type like __ieee128 which could be easily done by
> lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> "__ieee128");
> lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> "__float128");
> perhaps under some conditions, rethink if the -mfloat128 option exists
> and what it does etc., it can be done incrementally and the rs6000-c.cc
> hunks in the patch can be easily reverted (appart from the formatting
> fix).
There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
Sorry I did not pick up on that earlier.
The basic types, that always exist if they are supported at all, are
__ibm128 and __ieee128. Long double picks one of those, or some other
type; and __float128 is a source of confusion (it tries to make things
more similar to x86, but things are *not* similar anyway :-( )
Any code that cares about the actual mode used, or that supports more
than one type, should use the explicit types. Code that just wants what
"long double" stands for can just use that, of course.
> Bootstrapped/regtested on powerpc{64le,64}-linux, on powerpc64-linux
> with -m32/-m64 testing, ok for trunk?
Tested with which -mcpu=? You need at least power7 to get __ieee128
support, but it should be tested *without* that as well. (The actual
default for powerpc64-linux is power4 (so not even 970), and for
powerpc-linux it is 603 or such.)
> * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> __SIZEOF_FLOAT128__ here and only if __float128 macro is defined.
(if and only if?)
> * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> 85 instead of 86.
This should be auto-generated, or just not exist at all
("sizeof type_map / sizeof type_map[0]" in the one place it is used,
instead -- "one place" is after removing it from the definition of the
array of course, where it is unneeded :-) )
> static
> const char *rs6000_type_string (tree type_node)
> {
> + if (type_node == NULL_TREE)
> + return "unknown";
Please say this is NULL? If you want to indicate it is problematic, you
can say "***NULL***" or such?
> + { "if", "ibm128_float_type_node "
> + "? ibm128_float_type_node "
> + ": long_double" },
I don't think that is correct. If there is no __ibm128, there is no
IFmode either (they are two sides of the same coin). Using DFmode
instead (which is the only thing that "long double" could be if not
IFmode) will just ICE later, if this is used at all. So best case this
will confuse the reader.
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-07 21:37 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT, IBM}128__ " Segher Boessenkool
@ 2022-03-09 13:27 ` Jakub Jelinek
2022-03-09 14:00 ` Jonathan Wakely
2022-03-09 18:34 ` Segher Boessenkool
2022-03-10 20:44 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ " Michael Meissner
1 sibling, 2 replies; 13+ messages in thread
From: Jakub Jelinek @ 2022-03-09 13:27 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
> > macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
> > is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
> > actually supported or not.
> >
> > The following patch:
> > 1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
> > really support them instead of equal to long_double_type_node
> > 2) adjusts the builtins code to use
> > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > for the 2 builtins, so that we don't ICE during builtins creation
> > if ibm128_float_type_node is NULL (using error_mark_node instead of
> > long_double_type_node sounds more risky to me)
>
> I feel the opposite way: (potentially) using the wrong thing is just a
> ticking time bomb, never "safer".
Actually, fallback to long_double_type_node is the right thing, see below.
> There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
Ok, added now.
> The basic types, that always exist if they are supported at all, are
> __ibm128 and __ieee128. Long double picks one of those, or some other
> type; and __float128 is a source of confusion (it tries to make things
> more similar to x86, but things are *not* similar anyway :-( )
Not just x86, there are multiple targets with __float128 support, and
when it works, it behaves the same on all of them.
Mike's PR99708 patch (which unfortunately has been backported to various
branches without resolving these issues first) regressed on powerpc64-linux
+FAIL: gcc.dg/pr83198.c (test for excess errors)
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c -O2 (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c -O2 compilation failed to produce executable
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c -O2 -flto -fno-use-linker-plugin -flto-partition=none compilation failed to produce executable
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects compilation failed to produce executable
Those are all tests that use __float128 if __SIZEOF_FLOAT128__ is defined.
> Tested with which -mcpu=? You need at least power7 to get __ieee128
> support, but it should be tested *without* that as well. (The actual
> default for powerpc64-linux is power4 (so not even 970), and for
> powerpc-linux it is 603 or such.)
../configure --enable-languages=c,c++,fortran,objc,obj-c++,lto,go --prefix=/home/jakub/GCC; make -j64 > LOG 2>&1 && make -j64 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}' > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1
i.e. the oldest supported one. On powerpc64le-linux also without any
--with- tuning, so power8.
> > * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> > __SIZEOF_FLOAT128__ here and only if __float128 macro is defined.
>
> (if and only if?)
Changed.
> > * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > 85 instead of 86.
>
> This should be auto-generated, or just not exist at all
> ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> instead -- "one place" is after removing it from the definition of the
> array of course, where it is unneeded :-) )
Unfortunately the generator doesn't use libiberty.h, so I couldn't use
ARRAY_SIZE. Just used sizeof / sizeof [0].
> > static
> > const char *rs6000_type_string (tree type_node)
> > {
> > + if (type_node == NULL_TREE)
> > + return "unknown";
>
> Please say this is NULL? If you want to indicate it is problematic, you
> can say "***NULL***" or such?
Done.
> > + { "if", "ibm128_float_type_node "
> > + "? ibm128_float_type_node "
> > + ": long_double" },
>
> I don't think that is correct. If there is no __ibm128, there is no
> IFmode either (they are two sides of the same coin). Using DFmode
> instead (which is the only thing that "long double" could be if not
> IFmode) will just ICE later, if this is used at all. So best case this
> will confuse the reader.
rs6000_expand_builtin has (but at an incorrect spot) code to remap
__builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood,
for 2 reasons:
1) __ibm128 type is only supported when VSX is enabled, seems the intent
was that those 2 builtins can be used interchangeably unless
long double is the same as __ieee128, in which case
__builtin_{,un}pack_longdouble errors and
__builtin_{,un}pack_ibm128 works and returns/takes __ibm128
2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in
effect, ibm128_float_type_node has TFmode rather than IFmode,
so we can't use the {,un}packif expanders but need {,un}packtf
instead
Which means the
ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
is actually the right thing (except for -mlong-double-64 but the
patch will reject those builtins in that case) - if
ibm128_float_type_node is NULL but long_double_type_node is TFmode,
we want __builtin_{,un}pack_ibm128 to work like
__builtin_{,un}pack_longdouble and use long double, if
ibm128_float_type_node is non-NULL and long_double_type_node is TFmode,
ibm128_float_type_node == long_double_type_node and again we want it to
work like __builtin_{,un}pack_longdouble and finally if
ibm128_float_type_node is non-NULL and long_double_type_node is KFmode,
it will use ibm128_float_type_node with IFmode.
E.g. C++
auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); }
double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); }
double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); }
used to be strangely accepted with -mlong-double-64 but did just a weird
thing (now is rejected), with -mlong-double-128 without VSX it used to ICE,
e.g. with
error: unrecognizable insn:
5 | }
| ^
(insn 9 8 10 2 (set (reg:IF 119)
(unspec:TF [
(reg:DF 120)
(reg:DF 122)
] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1
(nil))
and now works, ditto used to ICE with -mlong-double-128 -mcpu=power8 etc.,
and just worked and keeps working with -mlong-double-128 -mabi=ieeelongdouble.
Ok for trunk if it passes another bootstrap/regtest on powerpc64{,le}-linux?
2022-03-09 Jakub Jelinek <jakub@redhat.com>
PR target/99708
* config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Remove
RS6000_BTI_ptr_ieee128_float and RS6000_BTI_ptr_ibm128_float.
(ptr_ieee128_float_type_node, ptr_ibm128_float_type_node): Remove.
* config/rs6000/rs6000-builtin.cc (rs6000_type_string): Return
"**NULL**" if type_node is NULL first. Handle
ieee128_float_type_node.
(rs6000_init_builtins): Don't initialize ptr_ieee128_float_type_node
and ptr_ibm128_float_type_node. Set ibm128_float_type_node and
ieee128_float_type_node to NULL rather than long_double_type_node if
they aren't supported.
(rs6000_expand_builtin): Error if bif_is_ibm128 and
!ibm128_float_type_node and !FLOAT128_2REG_P (TFmode). Remap
RS6000_BIF_{,UN}PACK_IF to RS6000_BIF_{,UN}PACK_TF much earlier
and only use bif_is_ibm128 check for it.
* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
__SIZEOF_FLOAT128__ here and only iff __float128 macro is defined.
(rs6000_cpu_cpp_builtins): Don't define __SIZEOF_FLOAT128__ here.
Define __SIZEOF_IBM128__=16 if ieee128_float_type_node is non-NULL.
Formatting fix.
* config/rs6000/rs6000-gen-builtins.cc: Document ibm128 attribute.
(struct attrinfo): Add isibm128 member.
(TYPE_MAP_SIZE): Remove.
(type_map): Use [] instead of [TYPE_MAP_SIZE]. For "if" use
ibm128_float_type_node only if it is non-NULL, otherwise fall back
to long_double_type_node. Remove "pif" entry.
(parse_bif_attrs): Handle ibm128 attribute and print it for debugging.
(write_decls): Output bif_ibm128_bit and bif_is_ibm128.
(write_type_node): Use sizeof (type_map) / sizeof (type_map[0])
instead of TYPE_MAP_SIZE.
(write_bif_static_init): Handle isibm128.
* config/rs6000/rs6000-builtins.def: Document ibm128 attribute.
(__builtin_pack_ibm128, __builtin_unpack_ibm128): Add ibm128
attribute.
* gcc.dg/pr99708.c: New test.
* gcc.target/powerpc/pr99708-2.c: New test.
--- gcc/config/rs6000/rs6000.h.jj 2022-03-05 11:42:42.758878256 +0100
+++ gcc/config/rs6000/rs6000.h 2022-03-09 12:01:02.564696908 +0100
@@ -2444,8 +2444,6 @@ enum rs6000_builtin_type_index
RS6000_BTI_ptr_long_double,
RS6000_BTI_ptr_dfloat64,
RS6000_BTI_ptr_dfloat128,
- RS6000_BTI_ptr_ieee128_float,
- RS6000_BTI_ptr_ibm128_float,
RS6000_BTI_ptr_vector_pair,
RS6000_BTI_ptr_vector_quad,
RS6000_BTI_ptr_long_long,
@@ -2541,8 +2539,6 @@ enum rs6000_builtin_type_index
#define ptr_long_double_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_double])
#define ptr_dfloat64_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat64])
#define ptr_dfloat128_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat128])
-#define ptr_ieee128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ieee128_float])
-#define ptr_ibm128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ibm128_float])
#define ptr_vector_pair_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_pair])
#define ptr_vector_quad_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_quad])
#define ptr_long_long_integer_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_long])
--- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-05 11:42:42.680879351 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 12:57:10.132121510 +0100
@@ -402,7 +402,9 @@ rs6000_vector_type (const char *name, tr
static
const char *rs6000_type_string (tree type_node)
{
- if (type_node == void_type_node)
+ if (type_node == NULL_TREE)
+ return "**NULL**";
+ else if (type_node == void_type_node)
return "void";
else if (type_node == long_integer_type_node)
return "long";
@@ -432,6 +434,8 @@ const char *rs6000_type_string (tree typ
return "ss";
else if (type_node == ibm128_float_type_node)
return "__ibm128";
+ else if (type_node == ieee128_float_type_node)
+ return "__ieee128";
else if (type_node == opaque_V4SI_type_node)
return "opaque";
else if (POINTER_TYPE_P (type_node))
@@ -721,7 +725,6 @@ rs6000_init_builtins (void)
layout_type (ibm128_float_type_node);
}
t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
- ptr_ibm128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ibm128_float_type_node,
"__ibm128");
@@ -730,13 +733,11 @@ rs6000_init_builtins (void)
else
ieee128_float_type_node = float128_type_node;
t = build_qualified_type (ieee128_float_type_node, TYPE_QUAL_CONST);
- ptr_ieee128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__ieee128");
}
-
else
- ieee128_float_type_node = ibm128_float_type_node = long_double_type_node;
+ ieee128_float_type_node = ibm128_float_type_node = NULL_TREE;
/* Vector pair and vector quad support. */
vector_pair_type_node = make_node (OPAQUE_TYPE);
@@ -3418,6 +3419,14 @@ rs6000_expand_builtin (tree exp, rtx tar
return const0_rtx;
}
+ if (bif_is_ibm128 (*bifaddr)
+ && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
+ {
+ error ("%qs requires %<__ibm128%> type support or %<long double%> "
+ "to be IBM 128-bit format", bifaddr->bifname);
+ return const0_rtx;
+ }
+
if (bif_is_cpu (*bifaddr))
return cpu_expand_builtin (fcode, exp, target);
@@ -3498,6 +3507,21 @@ rs6000_expand_builtin (tree exp, rtx tar
gcc_unreachable ();
}
+ if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
+ {
+ if (fcode == RS6000_BIF_PACK_IF)
+ {
+ icode = CODE_FOR_packtf;
+ fcode = RS6000_BIF_PACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ else if (fcode == RS6000_BIF_UNPACK_IF)
+ {
+ icode = CODE_FOR_unpacktf;
+ fcode = RS6000_BIF_UNPACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ }
/* TRUE iff the built-in function returns void. */
bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node;
@@ -3642,23 +3666,6 @@ rs6000_expand_builtin (tree exp, rtx tar
if (bif_is_mma (*bifaddr))
return mma_expand_builtin (exp, target, icode, fcode);
- if (fcode == RS6000_BIF_PACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_packtf;
- fcode = RS6000_BIF_PACK_TF;
- uns_fcode = (size_t) fcode;
- }
- else if (fcode == RS6000_BIF_UNPACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_unpacktf;
- fcode = RS6000_BIF_UNPACK_TF;
- uns_fcode = (size_t) fcode;
- }
-
if (TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node)
target = NULL_RTX;
else if (target == 0
--- gcc/config/rs6000/rs6000-c.cc.jj 2022-03-05 11:42:42.705879000 +0100
+++ gcc/config/rs6000/rs6000-c.cc 2022-03-09 12:21:18.304878817 +0100
@@ -584,6 +584,10 @@ rs6000_target_modify_macros (bool define
rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
else
rs6000_define_or_undefine_macro (false, "__float128");
+ if (ieee128_float_type_node && define_p)
+ rs6000_define_or_undefine_macro (true, "__SIZEOF_FLOAT128__=16");
+ else
+ rs6000_define_or_undefine_macro (false, "__SIZEOF_FLOAT128__");
}
/* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
via the target attribute/pragma. */
@@ -623,11 +627,11 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfi
if (TARGET_FRSQRTES)
builtin_define ("__RSQRTEF__");
if (TARGET_FLOAT128_TYPE)
- builtin_define ("__FLOAT128_TYPE__");
+ builtin_define ("__FLOAT128_TYPE__");
if (ibm128_float_type_node)
builtin_define ("__SIZEOF_IBM128__=16");
if (ieee128_float_type_node)
- builtin_define ("__SIZEOF_FLOAT128__=16");
+ builtin_define ("__SIZEOF_IEEE128__=16");
#ifdef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
builtin_define ("__BUILTIN_CPU_SUPPORTS__");
#endif
--- gcc/config/rs6000/rs6000-gen-builtins.cc.jj 2022-03-05 11:42:42.731878635 +0100
+++ gcc/config/rs6000/rs6000-gen-builtins.cc 2022-03-09 12:41:00.500540825 +0100
@@ -93,6 +93,8 @@ along with GCC; see the file COPYING3.
lxvrze Needs special handling for load-rightmost, zero-extended
endian Needs special handling for endianness
ibmld Restrict usage to the case when TFmode is IBM-128
+ ibm128 Restrict usage to the case where __ibm128 is supported or
+ if ibmld
An example stanza might look like this:
@@ -392,6 +394,7 @@ struct attrinfo
bool islxvrze;
bool isendian;
bool isibmld;
+ bool isibm128;
};
/* Fields associated with a function prototype (bif or overload). */
@@ -492,8 +495,7 @@ struct typemap
maps tokens from a fntype string to a tree type. For example,
in "si_ftype_hi" we would map "si" to "intSI_type_node" and
map "hi" to "intHI_type_node". */
-#define TYPE_MAP_SIZE 86
-static typemap type_map[TYPE_MAP_SIZE] =
+static typemap type_map[] =
{
{ "bi", "bool_int" },
{ "bv16qi", "bool_V16QI" },
@@ -506,7 +508,9 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "df", "double" },
{ "di", "long_long_integer" },
{ "hi", "intHI" },
- { "if", "ibm128_float" },
+ { "if", "ibm128_float_type_node "
+ "? ibm128_float_type_node "
+ ": long_double" },
{ "ld", "long_double" },
{ "lg", "long_integer" },
{ "pbv16qi", "ptr_bool_V16QI" },
@@ -519,7 +523,6 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "pdf", "ptr_double" },
{ "pdi", "ptr_long_long_integer" },
{ "phi", "ptr_intHI" },
- { "pif", "ptr_ibm128_float" },
{ "pld", "ptr_long_double" },
{ "plg", "ptr_long_integer" },
{ "pqi", "ptr_intQI" },
@@ -1439,6 +1442,8 @@ parse_bif_attrs (attrinfo *attrptr)
attrptr->isendian = 1;
else if (!strcmp (attrname, "ibmld"))
attrptr->isibmld = 1;
+ else if (!strcmp (attrname, "ibm128"))
+ attrptr->isibm128 = 1;
else
{
diag (oldpos, "unknown attribute.\n");
@@ -1472,14 +1477,15 @@ parse_bif_attrs (attrinfo *attrptr)
"ldvec = %d, stvec = %d, reve = %d, pred = %d, htm = %d, "
"htmspr = %d, htmcr = %d, mma = %d, quad = %d, pair = %d, "
"mmaint = %d, no32bit = %d, 32bit = %d, cpu = %d, ldstmask = %d, "
- "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld= %d.\n",
+ "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld = %d, ibm128 = %d.\n",
attrptr->isinit, attrptr->isset, attrptr->isextract,
attrptr->isnosoft, attrptr->isldvec, attrptr->isstvec,
attrptr->isreve, attrptr->ispred, attrptr->ishtm, attrptr->ishtmspr,
attrptr->ishtmcr, attrptr->ismma, attrptr->isquad, attrptr->ispair,
attrptr->ismmaint, attrptr->isno32bit, attrptr->is32bit,
attrptr->iscpu, attrptr->isldstmask, attrptr->islxvrse,
- attrptr->islxvrze, attrptr->isendian, attrptr->isibmld);
+ attrptr->islxvrze, attrptr->isendian, attrptr->isibmld,
+ attrptr->isibm128);
#endif
return PC_OK;
@@ -2294,6 +2300,7 @@ write_decls (void)
fprintf (header_file, "#define bif_lxvrze_bit\t\t(0x00100000)\n");
fprintf (header_file, "#define bif_endian_bit\t\t(0x00200000)\n");
fprintf (header_file, "#define bif_ibmld_bit\t\t(0x00400000)\n");
+ fprintf (header_file, "#define bif_ibm128_bit\t\t(0x00800000)\n");
fprintf (header_file, "\n");
fprintf (header_file,
"#define bif_is_init(x)\t\t((x).bifattrs & bif_init_bit)\n");
@@ -2341,6 +2348,8 @@ write_decls (void)
"#define bif_is_endian(x)\t((x).bifattrs & bif_endian_bit)\n");
fprintf (header_file,
"#define bif_is_ibmld(x)\t((x).bifattrs & bif_ibmld_bit)\n");
+ fprintf (header_file,
+ "#define bif_is_ibm128(x)\t((x).bifattrs & bif_ibm128_bit)\n");
fprintf (header_file, "\n");
fprintf (header_file,
@@ -2385,8 +2394,10 @@ write_type_node (char *tok, bool indent)
{
if (indent)
fprintf (init_file, " ");
- typemap *entry = (typemap *) bsearch (tok, type_map, TYPE_MAP_SIZE,
- sizeof (typemap), typemap_cmp);
+ typemap *entry
+ = (typemap *) bsearch (tok, type_map,
+ sizeof (type_map) / sizeof (type_map[0]),
+ sizeof (typemap), typemap_cmp);
if (!entry)
fatal ("Type map is inconsistent.");
fprintf (init_file, "%s_type_node", entry->value);
@@ -2535,6 +2546,8 @@ write_bif_static_init (void)
fprintf (init_file, " | bif_endian_bit");
if (bifp->attrs.isibmld)
fprintf (init_file, " | bif_ibmld_bit");
+ if (bifp->attrs.isibm128)
+ fprintf (init_file, " | bif_ibm128_bit");
fprintf (init_file, ",\n");
fprintf (init_file, " /* restr_opnd */\t{%d, %d, %d},\n",
bifp->proto.restr_opnd[0], bifp->proto.restr_opnd[1],
--- gcc/config/rs6000/rs6000-builtins.def.jj 2022-02-09 20:13:51.519305161 +0100
+++ gcc/config/rs6000/rs6000-builtins.def 2022-03-09 12:40:29.505969588 +0100
@@ -138,6 +138,7 @@
; lxvrze Needs special handling for load-rightmost, zero-extended
; endian Needs special handling for endianness
; ibmld Restrict usage to the case when TFmode is IBM-128
+; ibm128 Restrict usage to the case where __ibm128 is supported or if ibmld
;
; Each attribute corresponds to extra processing required when
; the built-in is expanded. All such special processing should
@@ -234,13 +235,13 @@
MTFSF rs6000_mtfsf {}
const __ibm128 __builtin_pack_ibm128 (double, double);
- PACK_IF packif {}
+ PACK_IF packif {ibm128}
void __builtin_set_fpscr_rn (const int[0,3]);
SET_FPSCR_RN rs6000_set_fpscr_rn {}
const double __builtin_unpack_ibm128 (__ibm128, const int<1>);
- UNPACK_IF unpackif {}
+ UNPACK_IF unpackif {ibm128}
; This is redundant with __builtin_unpack_ibm128, as it requires long
; double to be __ibm128. Should probably be deprecated.
--- gcc/testsuite/gcc.dg/pr99708.c.jj 2022-03-09 12:01:02.567696866 +0100
+++ gcc/testsuite/gcc.dg/pr99708.c 2022-03-09 12:01:02.567696866 +0100
@@ -0,0 +1,7 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_FLOAT128__
+__float128 f = 1.0;
+#endif
+long double l = 1.0;
--- gcc/testsuite/gcc.target/powerpc/pr99708-2.c.jj 2022-03-09 12:01:02.567696866 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr99708-2.c 2022-03-09 13:23:35.803146778 +0100
@@ -0,0 +1,22 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_IBM128__
+__ibm128 f = 1.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+__ieee128 g = 1.0;
+#endif
+long double h = 1.0;
+
+void
+foo (void)
+{
+#ifdef __SIZEOF_IBM128__
+ f += 2.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+ g += 2.0;
+#endif
+ l += 2.0;
+}
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 13:27 ` [PATCH] rs6000, v2: " Jakub Jelinek
@ 2022-03-09 14:00 ` Jonathan Wakely
2022-03-09 18:34 ` Segher Boessenkool
1 sibling, 0 replies; 13+ messages in thread
From: Jonathan Wakely @ 2022-03-09 14:00 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Segher Boessenkool, Michael Meissner, gcc Patches, David Edelsohn
On Wed, 9 Mar 2022 at 13:27, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > > * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > > 85 instead of 86.
> >
> > This should be auto-generated, or just not exist at all
> > ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> > instead -- "one place" is after removing it from the definition of the
> > array of course, where it is unneeded :-) )
>
> Unfortunately the generator doesn't use libiberty.h, so I couldn't use
> ARRAY_SIZE. Just used sizeof / sizeof [0].
FWIW this could be simply std::size(type_map) in C++17.
C++11 allows std::end(type_map) - std::begin(type_map) but that's not
really an improvement.
Those functions are all overloaded for arrays and deduce the array length.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 13:27 ` [PATCH] rs6000, v2: " Jakub Jelinek
2022-03-09 14:00 ` Jonathan Wakely
@ 2022-03-09 18:34 ` Segher Boessenkool
2022-03-09 19:24 ` Jakub Jelinek
1 sibling, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2022-03-09 18:34 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
Hi!
On Wed, Mar 09, 2022 at 02:27:19PM +0100, Jakub Jelinek wrote:
> On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > > 2) adjusts the builtins code to use
> > > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > > for the 2 builtins, so that we don't ICE during builtins creation
> > > if ibm128_float_type_node is NULL (using error_mark_node instead of
> > > long_double_type_node sounds more risky to me)
> >
> > I feel the opposite way: (potentially) using the wrong thing is just a
> > ticking time bomb, never "safer".
>
> Actually, fallback to long_double_type_node is the right thing, see below.
I don't see how that ever could be true, but I'll see below perhaps :-)
> > There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
>
> Ok, added now.
Thanks!
> > The basic types, that always exist if they are supported at all, are
> > __ibm128 and __ieee128. Long double picks one of those, or some other
> > type; and __float128 is a source of confusion (it tries to make things
> > more similar to x86, but things are *not* similar anyway :-( )
>
> Not just x86, there are multiple targets with __float128 support, and
> when it works, it behaves the same on all of them.
Unfortunately it isn't documented anywhere *what* it should mean, then.
> Mike's PR99708 patch (which unfortunately has been backported to various
> branches without resolving these issues first) regressed on powerpc64-linux
It supposedly was tested there though? Sigh.
> > Tested with which -mcpu=? You need at least power7 to get __ieee128
> > support, but it should be tested *without* that as well. (The actual
> > default for powerpc64-linux is power4 (so not even 970), and for
> > powerpc-linux it is 603 or such.)
>
> ../configure --enable-languages=c,c++,fortran,objc,obj-c++,lto,go --prefix=/home/jakub/GCC; make -j64 > LOG 2>&1 && make -j64 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}' > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1
> i.e. the oldest supported one. On powerpc64le-linux also without any
> --with- tuning, so power8.
So power4 for powerpc64-linux, and power8 for powerpc64le-linux.
I ask because many people do use --with-cpu=, and the difference matters
a lot here.
> > > * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > > 85 instead of 86.
> >
> > This should be auto-generated, or just not exist at all
> > ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> > instead -- "one place" is after removing it from the definition of the
> > array of course, where it is unneeded :-) )
>
> Unfortunately the generator doesn't use libiberty.h, so I couldn't use
> ARRAY_SIZE. Just used sizeof / sizeof [0].
It is a common idiom anyway, much clearer than any macro :-) (But no
parens please? sizeof is an operator, not a function).
> > > + { "if", "ibm128_float_type_node "
> > > + "? ibm128_float_type_node "
> > > + ": long_double" },
> >
> > I don't think that is correct. If there is no __ibm128, there is no
> > IFmode either (they are two sides of the same coin). Using DFmode
> > instead (which is the only thing that "long double" could be if not
> > IFmode) will just ICE later, if this is used at all. So best case this
> > will confuse the reader.
>
> rs6000_expand_builtin has (but at an incorrect spot) code to remap
> __builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood,
> for 2 reasons:
> 1) __ibm128 type is only supported when VSX is enabled,
Ugh, another bug. *^*ET*(^(*^TE($^TW(*T(W
> seems the intent
> was that those 2 builtins can be used interchangeably unless
> long double is the same as __ieee128, in which case
> __builtin_{,un}pack_longdouble errors and
> __builtin_{,un}pack_ibm128 works and returns/takes __ibm128
Aha.
But the type "if" always means the same thing, in the builtins language.
> 2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in
> effect, ibm128_float_type_node has TFmode rather than IFmode,
> so we can't use the {,un}packif expanders but need {,un}packtf
> instead
TFmode *is* IFmode, in that case. TFmode is a workaround for
shortcomings in the generic parts of GCC. This almost works as well
even! But it is confusing no end.
> Which means the
> ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> is actually the right thing (except for -mlong-double-64 but the
> patch will reject those builtins in that case) - if
> ibm128_float_type_node is NULL but long_double_type_node is TFmode,
> we want __builtin_{,un}pack_ibm128 to work like
> __builtin_{,un}pack_longdouble and use long double, if
> ibm128_float_type_node is non-NULL and long_double_type_node is TFmode,
> ibm128_float_type_node == long_double_type_node and again we want it to
> work like __builtin_{,un}pack_longdouble and finally if
> ibm128_float_type_node is non-NULL and long_double_type_node is KFmode,
> it will use ibm128_float_type_node with IFmode.
ibm128_float_type_node should always be defined if we can use double-
double. That is the whole point of having __ibm128 and __ieee128 types
at all: they exist whenever they can, and always mean the same thing.
We should never (have to) do strange gymnastics to access those types.
> E.g. C++
> auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); }
> double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); }
> double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); }
> used to be strangely accepted with -mlong-double-64 but did just a weird
> thing (now is rejected),
Neither is correct; it should do the simple and correct thing in both
cases. Whatever your long double is: long double has nothing whatsoever
to do with __ibm128 and __ieee128.
> with -mlong-double-128 without VSX it used to ICE,
> e.g. with
> error: unrecognizable insn:
> 5 | }
> | ^
> (insn 9 8 10 2 (set (reg:IF 119)
> (unspec:TF [
> (reg:DF 120)
> (reg:DF 122)
> ] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1
> (nil))
> and now works, ditto used to ICE with -mlong-double-128 -mcpu=power8 etc.,
> and just worked and keeps working with -mlong-double-128 -mabi=ieeelongdouble.
> + if (bif_is_ibm128 (*bifaddr)
> + && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
> + {
> + error ("%qs requires %<__ibm128%> type support or %<long double%> "
> + "to be IBM 128-bit format", bifaddr->bifname);
> + return const0_rtx;
> + }
So this part is wrong imo.
> + if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
> + {
> + if (fcode == RS6000_BIF_PACK_IF)
> + {
> + icode = CODE_FOR_packtf;
> + fcode = RS6000_BIF_PACK_TF;
> + uns_fcode = (size_t) fcode;
> + }
> + else if (fcode == RS6000_BIF_UNPACK_IF)
> + {
> + icode = CODE_FOR_unpacktf;
> + fcode = RS6000_BIF_UNPACK_TF;
> + uns_fcode = (size_t) fcode;
> + }
> + }
And this.
Thank you for dealing with this jumble!
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 18:34 ` Segher Boessenkool
@ 2022-03-09 19:24 ` Jakub Jelinek
2022-03-09 20:57 ` Segher Boessenkool
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2022-03-09 19:24 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Wed, Mar 09, 2022 at 12:34:06PM -0600, Segher Boessenkool wrote:
> It is a common idiom anyway, much clearer than any macro :-) (But no
> parens please? sizeof is an operator, not a function).
Ok, changed in my copy.
> > > > + { "if", "ibm128_float_type_node "
> > > > + "? ibm128_float_type_node "
> > > > + ": long_double" },
> > >
> > > I don't think that is correct. If there is no __ibm128, there is no
> > > IFmode either (they are two sides of the same coin). Using DFmode
> > > instead (which is the only thing that "long double" could be if not
> > > IFmode) will just ICE later, if this is used at all. So best case this
> > > will confuse the reader.
> >
> > rs6000_expand_builtin has (but at an incorrect spot) code to remap
> > __builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood,
> > for 2 reasons:
> > 1) __ibm128 type is only supported when VSX is enabled,
>
> Ugh, another bug. *^*ET*(^(*^TE($^TW(*T(W
I agree, but I don't have much energy to fix all the bugs in the backend in one
patch.
Probably it could be fixed with incremental:
--- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-09 19:55:31.879255798 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 20:10:53.778612345 +0100
@@ -713,9 +713,9 @@ rs6000_init_builtins (void)
For IEEE 128-bit floating point, always create the type __ieee128. If the
user used -mfloat128, rs6000-c.cc will create a define from __float128 to
__ieee128. */
- if (TARGET_FLOAT128_TYPE)
+ if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE))
{
- if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
+ if (!TARGET_IEEEQUAD)
ibm128_float_type_node = long_double_type_node;
else
{
@@ -727,7 +727,12 @@ rs6000_init_builtins (void)
t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
lang_hooks.types.register_builtin_type (ibm128_float_type_node,
"__ibm128");
+ }
+ else
+ ibm128_float_type_node = NULL_TREE;
+ if (TARGET_FLOAT128_TYPE)
+ {
if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
ieee128_float_type_node = long_double_type_node;
else
@@ -737,7 +742,7 @@ rs6000_init_builtins (void)
"__ieee128");
}
else
- ieee128_float_type_node = ibm128_float_type_node = NULL_TREE;
+ ieee128_float_type_node = NULL_TREE;
/* Vector pair and vector quad support. */
vector_pair_type_node = make_node (OPAQUE_TYPE);
@@ -3419,11 +3424,10 @@ rs6000_expand_builtin (tree exp, rtx tar
return const0_rtx;
}
- if (bif_is_ibm128 (*bifaddr)
- && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
+ if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node)
{
- error ("%qs requires %<__ibm128%> type support or %<long double%> "
- "to be IBM 128-bit format", bifaddr->bifname);
+ error ("%qs requires %<__ibm128%> type support",
+ bifaddr->bifname);
return const0_rtx;
}
which ought to support __ibm128 as the same thing as long double
if long double is double double, and also as a separate IFmode
double double if -mabi=ieeelongdouble and TARGET_FLOAT128_TYPE
(i.e. both IFmode and KFmode are supported).
> > seems the intent
> > was that those 2 builtins can be used interchangeably unless
> > long double is the same as __ieee128, in which case
> > __builtin_{,un}pack_longdouble errors and
> > __builtin_{,un}pack_ibm128 works and returns/takes __ibm128
>
> Aha.
>
> But the type "if" always means the same thing, in the builtins language.
True. Before the patch it means ibm128_float_type_node, which is
a tree connected to __ibm128 type if it exists (but could very well be
equal to long_double_type_node) or long_double_type if it doesn't.
With the patch it means the same thing, except that ibm128_float_type_node
is NULL if __ibm128 isn't supported, and at that point if means
long_double_type_node. So, on the builtins side, if is the same thing
as before.
> > 2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in
> > effect, ibm128_float_type_node has TFmode rather than IFmode,
> > so we can't use the {,un}packif expanders but need {,un}packtf
> > instead
>
> TFmode *is* IFmode, in that case. TFmode is a workaround for
> shortcomings in the generic parts of GCC. This almost works as well
> even! But it is confusing no end.
TFmode isn't IFmode in that case, otherwise there wouldn't be the ICE on
(insn 9 8 10 2 (set (reg:IF 119)
(unspec:TF [
(reg:DF 120)
(reg:DF 122)
] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1
(nil))
etc. IFmode and TFmode clearly are separate modes, which in those
cases have the same REAL_MODE_FORMAT. So, under the hood they are handled
the same, but still it isn't acceptable in RTL to mix the two modes
in cases where RTL requires the same mode. Like in the SET_DEST/SET_SRC
case must have the same mode, or operands of most arithmetic ops and the
result have to have the same mode, ...
> > Which means the
> > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > is actually the right thing (except for -mlong-double-64 but the
> > patch will reject those builtins in that case) - if
> > ibm128_float_type_node is NULL but long_double_type_node is TFmode,
> > we want __builtin_{,un}pack_ibm128 to work like
> > __builtin_{,un}pack_longdouble and use long double, if
> > ibm128_float_type_node is non-NULL and long_double_type_node is TFmode,
> > ibm128_float_type_node == long_double_type_node and again we want it to
> > work like __builtin_{,un}pack_longdouble and finally if
> > ibm128_float_type_node is non-NULL and long_double_type_node is KFmode,
> > it will use ibm128_float_type_node with IFmode.
>
> ibm128_float_type_node should always be defined if we can use double-
> double. That is the whole point of having __ibm128 and __ieee128 types
> at all: they exist whenever they can, and always mean the same thing.
> We should never (have to) do strange gymnastics to access those types.
With the above incremental patch, perhaps (if it works).
But still we can't make if be NULL if __ibm128 isn't supported, because
we'll ICE already during creation of the builtins.
Perhaps error_mark_node might be a possibility (though very risky), but
because of the builtins generator that isn't an option (it appends
"_type_node" and error_mark_node doesn't end with "_type_node").
Using long double will keep the existing behavior for that case and
we'll reject calls to the builtin.
> > E.g. C++
> > auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); }
> > double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); }
> > double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); }
> > used to be strangely accepted with -mlong-double-64 but did just a weird
> > thing (now is rejected),
>
> Neither is correct; it should do the simple and correct thing in both
> cases. Whatever your long double is: long double has nothing whatsoever
> to do with __ibm128 and __ieee128.
With the above incremental patch I could use there __ibm128 instead of
long double in the testcase (but even if I don't, there is just
an extra long double to __ibm128 conversion which is either nop
conversion or __float128 to __ibm128 conversion).
What I wanted to say is that with e.g. -mlong-double-64 when there
just isn't any double double there is nothing to pack or unpack,
we should reject it rather than doing something weird.
> > + if (bif_is_ibm128 (*bifaddr)
> > + && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
> > + {
> > + error ("%qs requires %<__ibm128%> type support or %<long double%> "
> > + "to be IBM 128-bit format", bifaddr->bifname);
> > + return const0_rtx;
> > + }
>
> So this part is wrong imo.
The incremental patch tweaks this.
> > + if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
> > + {
> > + if (fcode == RS6000_BIF_PACK_IF)
> > + {
> > + icode = CODE_FOR_packtf;
> > + fcode = RS6000_BIF_PACK_TF;
> > + uns_fcode = (size_t) fcode;
> > + }
> > + else if (fcode == RS6000_BIF_UNPACK_IF)
> > + {
> > + icode = CODE_FOR_unpacktf;
> > + fcode = RS6000_BIF_UNPACK_TF;
> > + uns_fcode = (size_t) fcode;
> > + }
> > + }
>
> And this.
This is really necessary. I could check
&& TYPE_MODE (ibm128_float_type_node) == TFmode
with the incremental patch if that is better understandable, but still, due
to TFmode != IFmode it is required, and after all, it has been there
before too, just done too late so that it already did a wrong thing before.
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 19:24 ` Jakub Jelinek
@ 2022-03-09 20:57 ` Segher Boessenkool
2022-03-09 21:10 ` [PATCH] rs6000, v3: " Jakub Jelinek
0 siblings, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2022-03-09 20:57 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Wed, Mar 09, 2022 at 08:24:24PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2022 at 12:34:06PM -0600, Segher Boessenkool wrote:
> > > rs6000_expand_builtin has (but at an incorrect spot) code to remap
> > > __builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood,
> > > for 2 reasons:
> > > 1) __ibm128 type is only supported when VSX is enabled,
> >
> > Ugh, another bug. *^*ET*(^(*^TE($^TW(*T(W
>
> I agree, but I don't have much energy to fix all the bugs in the backend in one
> patch.
Yes, but this is a very fundamental thing that causes a lot of
unnecessary complexity :-(
> Probably it could be fixed with incremental:
>
> --- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-09 19:55:31.879255798 +0100
> +++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 20:10:53.778612345 +0100
> @@ -713,9 +713,9 @@ rs6000_init_builtins (void)
> For IEEE 128-bit floating point, always create the type __ieee128. If the
> user used -mfloat128, rs6000-c.cc will create a define from __float128 to
> __ieee128. */
> - if (TARGET_FLOAT128_TYPE)
> + if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE))
> {
> - if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
> + if (!TARGET_IEEEQUAD)
> ibm128_float_type_node = long_double_type_node;
> else
> {
> @@ -727,7 +727,12 @@ rs6000_init_builtins (void)
> t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
> lang_hooks.types.register_builtin_type (ibm128_float_type_node,
> "__ibm128");
> + }
> + else
> + ibm128_float_type_node = NULL_TREE;
>
> + if (TARGET_FLOAT128_TYPE)
> + {
> if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
> ieee128_float_type_node = long_double_type_node;
> else
> @@ -737,7 +742,7 @@ rs6000_init_builtins (void)
> "__ieee128");
> }
> else
> - ieee128_float_type_node = ibm128_float_type_node = NULL_TREE;
> + ieee128_float_type_node = NULL_TREE;
>
> /* Vector pair and vector quad support. */
> vector_pair_type_node = make_node (OPAQUE_TYPE);
> @@ -3419,11 +3424,10 @@ rs6000_expand_builtin (tree exp, rtx tar
> return const0_rtx;
> }
>
> - if (bif_is_ibm128 (*bifaddr)
> - && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
> + if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node)
> {
> - error ("%qs requires %<__ibm128%> type support or %<long double%> "
> - "to be IBM 128-bit format", bifaddr->bifname);
> + error ("%qs requires %<__ibm128%> type support",
> + bifaddr->bifname);
> return const0_rtx;
> }
Something like that yes. It still is all way more complicated than it
should be. Not your fault of course.
> which ought to support __ibm128 as the same thing as long double
> if long double is double double, and also as a separate IFmode
> double double if -mabi=ieeelongdouble and TARGET_FLOAT128_TYPE
> (i.e. both IFmode and KFmode are supported).
It should support __ibm128 always, and __ieee128 whenever VSX is
supported (it should be whenever VMX is supported, and even *always* as
well imnsho, but I haven't won that fight yet).
> > > seems the intent
> > > was that those 2 builtins can be used interchangeably unless
> > > long double is the same as __ieee128, in which case
> > > __builtin_{,un}pack_longdouble errors and
> > > __builtin_{,un}pack_ibm128 works and returns/takes __ibm128
> >
> > Aha.
> >
> > But the type "if" always means the same thing, in the builtins language.
>
> True. Before the patch it means ibm128_float_type_node, which is
> a tree connected to __ibm128 type if it exists (but could very well be
> equal to long_double_type_node) or long_double_type if it doesn't.
> With the patch it means the same thing, except that ibm128_float_type_node
> is NULL if __ibm128 isn't supported, and at that point if means
> long_double_type_node. So, on the builtins side, if is the same thing
> as before.
Ah good.
> > > 2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in
> > > effect, ibm128_float_type_node has TFmode rather than IFmode,
> > > so we can't use the {,un}packif expanders but need {,un}packtf
> > > instead
> >
> > TFmode *is* IFmode, in that case. TFmode is a workaround for
> > shortcomings in the generic parts of GCC. This almost works as well
> > even! But it is confusing no end.
>
> TFmode isn't IFmode in that case, otherwise there wouldn't be the ICE on
> (insn 9 8 10 2 (set (reg:IF 119)
> (unspec:TF [
> (reg:DF 120)
> (reg:DF 122)
> ] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1
> (nil))
> etc. IFmode and TFmode clearly are separate modes, which in those
> cases have the same REAL_MODE_FORMAT. So, under the hood they are handled
> the same, but still it isn't acceptable in RTL to mix the two modes
> in cases where RTL requires the same mode. Like in the SET_DEST/SET_SRC
> case must have the same mode, or operands of most arithmetic ops and the
> result have to have the same mode, ...
Yes, for some strange reason that I still do not understand we do not
#define one mode to the other. Buy you agree TFmode is what IFmode is
otherwise... This stuff is too complicated to explain :-(
> > > Which means the
> > > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > > is actually the right thing (except for -mlong-double-64 but the
> > > patch will reject those builtins in that case) - if
> > > ibm128_float_type_node is NULL but long_double_type_node is TFmode,
> > > we want __builtin_{,un}pack_ibm128 to work like
> > > __builtin_{,un}pack_longdouble and use long double, if
> > > ibm128_float_type_node is non-NULL and long_double_type_node is TFmode,
> > > ibm128_float_type_node == long_double_type_node and again we want it to
> > > work like __builtin_{,un}pack_longdouble and finally if
> > > ibm128_float_type_node is non-NULL and long_double_type_node is KFmode,
> > > it will use ibm128_float_type_node with IFmode.
> >
> > ibm128_float_type_node should always be defined if we can use double-
> > double. That is the whole point of having __ibm128 and __ieee128 types
> > at all: they exist whenever they can, and always mean the same thing.
> > We should never (have to) do strange gymnastics to access those types.
>
> With the above incremental patch, perhaps (if it works).
The way it was *meant to be*. Not how it was implemented it seems :-(
> But still we can't make if be NULL if __ibm128 isn't supported, because
> we'll ICE already during creation of the builtins.
> Perhaps error_mark_node might be a possibility (though very risky), but
> because of the builtins generator that isn't an option (it appends
> "_type_node" and error_mark_node doesn't end with "_type_node").
> Using long double will keep the existing behavior for that case and
> we'll reject calls to the builtin.
But __ibm128 should *always* be supported, so this is a hypothetical
problem.
> > > E.g. C++
> > > auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); }
> > > double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); }
> > > double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); }
> > > used to be strangely accepted with -mlong-double-64 but did just a weird
> > > thing (now is rejected),
> >
> > Neither is correct; it should do the simple and correct thing in both
> > cases. Whatever your long double is: long double has nothing whatsoever
> > to do with __ibm128 and __ieee128.
>
> With the above incremental patch I could use there __ibm128 instead of
> long double in the testcase (but even if I don't, there is just
> an extra long double to __ibm128 conversion which is either nop
> conversion or __float128 to __ibm128 conversion).
> What I wanted to say is that with e.g. -mlong-double-64 when there
> just isn't any double double there is nothing to pack or unpack,
> we should reject it rather than doing something weird.
Ah. Yet Another bug? -mlong-double-64 should not mean at all that
double-double does not exist / is not meaningful, just that this isn't
what long double uses.
> > > + if (bif_is_ibm128 (*bifaddr)
> > > + && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
> > > + {
> > > + error ("%qs requires %<__ibm128%> type support or %<long double%> "
> > > + "to be IBM 128-bit format", bifaddr->bifname);
> > > + return const0_rtx;
> > > + }
> >
> > So this part is wrong imo.
>
> The incremental patch tweaks this.
>
> > > + if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
> > > + {
> > > + if (fcode == RS6000_BIF_PACK_IF)
> > > + {
> > > + icode = CODE_FOR_packtf;
> > > + fcode = RS6000_BIF_PACK_TF;
> > > + uns_fcode = (size_t) fcode;
> > > + }
> > > + else if (fcode == RS6000_BIF_UNPACK_IF)
> > > + {
> > > + icode = CODE_FOR_unpacktf;
> > > + fcode = RS6000_BIF_UNPACK_TF;
> > > + uns_fcode = (size_t) fcode;
> > > + }
> > > + }
> >
> > And this.
>
> This is really necessary. I could check
> && TYPE_MODE (ibm128_float_type_node) == TFmode
> with the incremental patch if that is better understandable, but still, due
> to TFmode != IFmode it is required, and after all, it has been there
> before too, just done too late so that it already did a wrong thing before.
If you are fed up with all this, please commit what you have now (after
testing of course ;-) ), and I'll pick up things myself. Either way,
thank you for all your work on this!
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] rs6000, v3: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 20:57 ` Segher Boessenkool
@ 2022-03-09 21:10 ` Jakub Jelinek
2022-03-09 22:57 ` Segher Boessenkool
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2022-03-09 21:10 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Wed, Mar 09, 2022 at 02:57:20PM -0600, Segher Boessenkool wrote:
> But __ibm128 should *always* be supported, so this is a hypothetical
> problem.
I bet that will require much more work. I think for the barely supported
(or really unsupported?) case of old sysv IEEE quad or for when long double
is just DFmode we'd need some more (IFmode?) for __ibm128 and deal with
everything it needs (mapping it to libgcc entrypoints etc.).
> If you are fed up with all this, please commit what you have now (after
> testing of course ;-) ), and I'll pick up things myself. Either way,
> thank you for all your work on this!
Ok, here is what I'll test momentarily:
2022-03-09 Jakub Jelinek <jakub@redhat.com>
PR target/99708
* config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Remove
RS6000_BTI_ptr_ieee128_float and RS6000_BTI_ptr_ibm128_float.
(ptr_ieee128_float_type_node, ptr_ibm128_float_type_node): Remove.
* config/rs6000/rs6000-builtin.cc (rs6000_type_string): Return
"**NULL**" if type_node is NULL first. Handle
ieee128_float_type_node.
(rs6000_init_builtins): Don't initialize ptr_ieee128_float_type_node
and ptr_ibm128_float_type_node. Set ibm128_float_type_node and
ieee128_float_type_node to NULL rather than long_double_type_node if
they aren't supported. Do support __ibm128 even if
!TARGET_FLOAT128_TYPE when long double is double double.
(rs6000_expand_builtin): Error if bif_is_ibm128 and
!ibm128_float_type_node. Remap RS6000_BIF_{,UN}PACK_IF to
RS6000_BIF_{,UN}PACK_TF much earlier and only use bif_is_ibm128 check
for it.
* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
__SIZEOF_FLOAT128__ here and only iff __float128 macro is defined.
(rs6000_cpu_cpp_builtins): Don't define __SIZEOF_FLOAT128__ here.
Define __SIZEOF_IBM128__=16 if ieee128_float_type_node is non-NULL.
Formatting fix.
* config/rs6000/rs6000-gen-builtins.cc: Document ibm128 attribute.
(struct attrinfo): Add isibm128 member.
(TYPE_MAP_SIZE): Remove.
(type_map): Use [] instead of [TYPE_MAP_SIZE]. For "if" use
ibm128_float_type_node only if it is non-NULL, otherwise fall back
to long_double_type_node. Remove "pif" entry.
(parse_bif_attrs): Handle ibm128 attribute and print it for debugging.
(write_decls): Output bif_ibm128_bit and bif_is_ibm128.
(write_type_node): Use sizeof type_map / sizeof type_map[0]
instead of TYPE_MAP_SIZE.
(write_bif_static_init): Handle isibm128.
* config/rs6000/rs6000-builtins.def: Document ibm128 attribute.
(__builtin_pack_ibm128, __builtin_unpack_ibm128): Add ibm128
attribute.
* gcc.dg/pr99708.c: New test.
* gcc.target/powerpc/pr99708-2.c: New test.
--- gcc/config/rs6000/rs6000.h.jj 2022-03-09 15:24:50.647017881 +0100
+++ gcc/config/rs6000/rs6000.h 2022-03-09 19:55:31.879255798 +0100
@@ -2444,8 +2444,6 @@ enum rs6000_builtin_type_index
RS6000_BTI_ptr_long_double,
RS6000_BTI_ptr_dfloat64,
RS6000_BTI_ptr_dfloat128,
- RS6000_BTI_ptr_ieee128_float,
- RS6000_BTI_ptr_ibm128_float,
RS6000_BTI_ptr_vector_pair,
RS6000_BTI_ptr_vector_quad,
RS6000_BTI_ptr_long_long,
@@ -2541,8 +2539,6 @@ enum rs6000_builtin_type_index
#define ptr_long_double_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_double])
#define ptr_dfloat64_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat64])
#define ptr_dfloat128_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat128])
-#define ptr_ieee128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ieee128_float])
-#define ptr_ibm128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ibm128_float])
#define ptr_vector_pair_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_pair])
#define ptr_vector_quad_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_quad])
#define ptr_long_long_integer_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_long])
--- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-09 15:24:50.642017950 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 20:10:53.778612345 +0100
@@ -402,7 +402,9 @@ rs6000_vector_type (const char *name, tr
static
const char *rs6000_type_string (tree type_node)
{
- if (type_node == void_type_node)
+ if (type_node == NULL_TREE)
+ return "**NULL**";
+ else if (type_node == void_type_node)
return "void";
else if (type_node == long_integer_type_node)
return "long";
@@ -432,6 +434,8 @@ const char *rs6000_type_string (tree typ
return "ss";
else if (type_node == ibm128_float_type_node)
return "__ibm128";
+ else if (type_node == ieee128_float_type_node)
+ return "__ieee128";
else if (type_node == opaque_V4SI_type_node)
return "opaque";
else if (POINTER_TYPE_P (type_node))
@@ -709,9 +713,9 @@ rs6000_init_builtins (void)
For IEEE 128-bit floating point, always create the type __ieee128. If the
user used -mfloat128, rs6000-c.cc will create a define from __float128 to
__ieee128. */
- if (TARGET_FLOAT128_TYPE)
+ if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE))
{
- if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
+ if (!TARGET_IEEEQUAD)
ibm128_float_type_node = long_double_type_node;
else
{
@@ -721,22 +725,24 @@ rs6000_init_builtins (void)
layout_type (ibm128_float_type_node);
}
t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
- ptr_ibm128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ibm128_float_type_node,
"__ibm128");
+ }
+ else
+ ibm128_float_type_node = NULL_TREE;
+ if (TARGET_FLOAT128_TYPE)
+ {
if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
ieee128_float_type_node = long_double_type_node;
else
ieee128_float_type_node = float128_type_node;
t = build_qualified_type (ieee128_float_type_node, TYPE_QUAL_CONST);
- ptr_ieee128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__ieee128");
}
-
else
- ieee128_float_type_node = ibm128_float_type_node = long_double_type_node;
+ ieee128_float_type_node = NULL_TREE;
/* Vector pair and vector quad support. */
vector_pair_type_node = make_node (OPAQUE_TYPE);
@@ -3418,6 +3424,13 @@ rs6000_expand_builtin (tree exp, rtx tar
return const0_rtx;
}
+ if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node)
+ {
+ error ("%qs requires %<__ibm128%> type support",
+ bifaddr->bifname);
+ return const0_rtx;
+ }
+
if (bif_is_cpu (*bifaddr))
return cpu_expand_builtin (fcode, exp, target);
@@ -3498,6 +3511,21 @@ rs6000_expand_builtin (tree exp, rtx tar
gcc_unreachable ();
}
+ if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
+ {
+ if (fcode == RS6000_BIF_PACK_IF)
+ {
+ icode = CODE_FOR_packtf;
+ fcode = RS6000_BIF_PACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ else if (fcode == RS6000_BIF_UNPACK_IF)
+ {
+ icode = CODE_FOR_unpacktf;
+ fcode = RS6000_BIF_UNPACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ }
/* TRUE iff the built-in function returns void. */
bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node;
@@ -3642,23 +3670,6 @@ rs6000_expand_builtin (tree exp, rtx tar
if (bif_is_mma (*bifaddr))
return mma_expand_builtin (exp, target, icode, fcode);
- if (fcode == RS6000_BIF_PACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_packtf;
- fcode = RS6000_BIF_PACK_TF;
- uns_fcode = (size_t) fcode;
- }
- else if (fcode == RS6000_BIF_UNPACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_unpacktf;
- fcode = RS6000_BIF_UNPACK_TF;
- uns_fcode = (size_t) fcode;
- }
-
if (TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node)
target = NULL_RTX;
else if (target == 0
--- gcc/config/rs6000/rs6000-c.cc.jj 2022-03-09 15:24:50.643017937 +0100
+++ gcc/config/rs6000/rs6000-c.cc 2022-03-09 19:55:31.880255784 +0100
@@ -584,6 +584,10 @@ rs6000_target_modify_macros (bool define
rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
else
rs6000_define_or_undefine_macro (false, "__float128");
+ if (ieee128_float_type_node && define_p)
+ rs6000_define_or_undefine_macro (true, "__SIZEOF_FLOAT128__=16");
+ else
+ rs6000_define_or_undefine_macro (false, "__SIZEOF_FLOAT128__");
}
/* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
via the target attribute/pragma. */
@@ -623,11 +627,11 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfi
if (TARGET_FRSQRTES)
builtin_define ("__RSQRTEF__");
if (TARGET_FLOAT128_TYPE)
- builtin_define ("__FLOAT128_TYPE__");
+ builtin_define ("__FLOAT128_TYPE__");
if (ibm128_float_type_node)
builtin_define ("__SIZEOF_IBM128__=16");
if (ieee128_float_type_node)
- builtin_define ("__SIZEOF_FLOAT128__=16");
+ builtin_define ("__SIZEOF_IEEE128__=16");
#ifdef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
builtin_define ("__BUILTIN_CPU_SUPPORTS__");
#endif
--- gcc/config/rs6000/rs6000-gen-builtins.cc.jj 2022-03-09 15:24:50.644017923 +0100
+++ gcc/config/rs6000/rs6000-gen-builtins.cc 2022-03-09 19:55:31.880255784 +0100
@@ -93,6 +93,8 @@ along with GCC; see the file COPYING3.
lxvrze Needs special handling for load-rightmost, zero-extended
endian Needs special handling for endianness
ibmld Restrict usage to the case when TFmode is IBM-128
+ ibm128 Restrict usage to the case where __ibm128 is supported or
+ if ibmld
An example stanza might look like this:
@@ -392,6 +394,7 @@ struct attrinfo
bool islxvrze;
bool isendian;
bool isibmld;
+ bool isibm128;
};
/* Fields associated with a function prototype (bif or overload). */
@@ -492,8 +495,7 @@ struct typemap
maps tokens from a fntype string to a tree type. For example,
in "si_ftype_hi" we would map "si" to "intSI_type_node" and
map "hi" to "intHI_type_node". */
-#define TYPE_MAP_SIZE 86
-static typemap type_map[TYPE_MAP_SIZE] =
+static typemap type_map[] =
{
{ "bi", "bool_int" },
{ "bv16qi", "bool_V16QI" },
@@ -506,7 +508,9 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "df", "double" },
{ "di", "long_long_integer" },
{ "hi", "intHI" },
- { "if", "ibm128_float" },
+ { "if", "ibm128_float_type_node "
+ "? ibm128_float_type_node "
+ ": long_double" },
{ "ld", "long_double" },
{ "lg", "long_integer" },
{ "pbv16qi", "ptr_bool_V16QI" },
@@ -519,7 +523,6 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "pdf", "ptr_double" },
{ "pdi", "ptr_long_long_integer" },
{ "phi", "ptr_intHI" },
- { "pif", "ptr_ibm128_float" },
{ "pld", "ptr_long_double" },
{ "plg", "ptr_long_integer" },
{ "pqi", "ptr_intQI" },
@@ -1439,6 +1442,8 @@ parse_bif_attrs (attrinfo *attrptr)
attrptr->isendian = 1;
else if (!strcmp (attrname, "ibmld"))
attrptr->isibmld = 1;
+ else if (!strcmp (attrname, "ibm128"))
+ attrptr->isibm128 = 1;
else
{
diag (oldpos, "unknown attribute.\n");
@@ -1472,14 +1477,15 @@ parse_bif_attrs (attrinfo *attrptr)
"ldvec = %d, stvec = %d, reve = %d, pred = %d, htm = %d, "
"htmspr = %d, htmcr = %d, mma = %d, quad = %d, pair = %d, "
"mmaint = %d, no32bit = %d, 32bit = %d, cpu = %d, ldstmask = %d, "
- "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld= %d.\n",
+ "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld = %d, ibm128 = %d.\n",
attrptr->isinit, attrptr->isset, attrptr->isextract,
attrptr->isnosoft, attrptr->isldvec, attrptr->isstvec,
attrptr->isreve, attrptr->ispred, attrptr->ishtm, attrptr->ishtmspr,
attrptr->ishtmcr, attrptr->ismma, attrptr->isquad, attrptr->ispair,
attrptr->ismmaint, attrptr->isno32bit, attrptr->is32bit,
attrptr->iscpu, attrptr->isldstmask, attrptr->islxvrse,
- attrptr->islxvrze, attrptr->isendian, attrptr->isibmld);
+ attrptr->islxvrze, attrptr->isendian, attrptr->isibmld,
+ attrptr->isibm128);
#endif
return PC_OK;
@@ -2294,6 +2300,7 @@ write_decls (void)
fprintf (header_file, "#define bif_lxvrze_bit\t\t(0x00100000)\n");
fprintf (header_file, "#define bif_endian_bit\t\t(0x00200000)\n");
fprintf (header_file, "#define bif_ibmld_bit\t\t(0x00400000)\n");
+ fprintf (header_file, "#define bif_ibm128_bit\t\t(0x00800000)\n");
fprintf (header_file, "\n");
fprintf (header_file,
"#define bif_is_init(x)\t\t((x).bifattrs & bif_init_bit)\n");
@@ -2341,6 +2348,8 @@ write_decls (void)
"#define bif_is_endian(x)\t((x).bifattrs & bif_endian_bit)\n");
fprintf (header_file,
"#define bif_is_ibmld(x)\t((x).bifattrs & bif_ibmld_bit)\n");
+ fprintf (header_file,
+ "#define bif_is_ibm128(x)\t((x).bifattrs & bif_ibm128_bit)\n");
fprintf (header_file, "\n");
fprintf (header_file,
@@ -2385,8 +2394,10 @@ write_type_node (char *tok, bool indent)
{
if (indent)
fprintf (init_file, " ");
- typemap *entry = (typemap *) bsearch (tok, type_map, TYPE_MAP_SIZE,
- sizeof (typemap), typemap_cmp);
+ typemap *entry
+ = (typemap *) bsearch (tok, type_map,
+ sizeof type_map / sizeof type_map[0],
+ sizeof (typemap), typemap_cmp);
if (!entry)
fatal ("Type map is inconsistent.");
fprintf (init_file, "%s_type_node", entry->value);
@@ -2535,6 +2546,8 @@ write_bif_static_init (void)
fprintf (init_file, " | bif_endian_bit");
if (bifp->attrs.isibmld)
fprintf (init_file, " | bif_ibmld_bit");
+ if (bifp->attrs.isibm128)
+ fprintf (init_file, " | bif_ibm128_bit");
fprintf (init_file, ",\n");
fprintf (init_file, " /* restr_opnd */\t{%d, %d, %d},\n",
bifp->proto.restr_opnd[0], bifp->proto.restr_opnd[1],
--- gcc/config/rs6000/rs6000-builtins.def.jj 2022-03-09 15:24:50.643017937 +0100
+++ gcc/config/rs6000/rs6000-builtins.def 2022-03-09 19:55:31.880255784 +0100
@@ -138,6 +138,7 @@
; lxvrze Needs special handling for load-rightmost, zero-extended
; endian Needs special handling for endianness
; ibmld Restrict usage to the case when TFmode is IBM-128
+; ibm128 Restrict usage to the case where __ibm128 is supported or if ibmld
;
; Each attribute corresponds to extra processing required when
; the built-in is expanded. All such special processing should
@@ -234,13 +235,13 @@
MTFSF rs6000_mtfsf {}
const __ibm128 __builtin_pack_ibm128 (double, double);
- PACK_IF packif {}
+ PACK_IF packif {ibm128}
void __builtin_set_fpscr_rn (const int[0,3]);
SET_FPSCR_RN rs6000_set_fpscr_rn {}
const double __builtin_unpack_ibm128 (__ibm128, const int<1>);
- UNPACK_IF unpackif {}
+ UNPACK_IF unpackif {ibm128}
; This is redundant with __builtin_unpack_ibm128, as it requires long
; double to be __ibm128. Should probably be deprecated.
--- gcc/testsuite/gcc.dg/pr99708.c.jj 2022-03-09 19:55:31.880255784 +0100
+++ gcc/testsuite/gcc.dg/pr99708.c 2022-03-09 19:55:31.880255784 +0100
@@ -0,0 +1,7 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_FLOAT128__
+__float128 f = 1.0;
+#endif
+long double l = 1.0;
--- gcc/testsuite/gcc.target/powerpc/pr99708-2.c.jj 2022-03-09 19:55:31.881255770 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr99708-2.c 2022-03-09 19:55:31.880255784 +0100
@@ -0,0 +1,22 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_IBM128__
+__ibm128 f = 1.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+__ieee128 g = 1.0;
+#endif
+long double h = 1.0;
+
+void
+foo (void)
+{
+#ifdef __SIZEOF_IBM128__
+ f += 2.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+ g += 2.0;
+#endif
+ l += 2.0;
+}
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v3: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 21:10 ` [PATCH] rs6000, v3: " Jakub Jelinek
@ 2022-03-09 22:57 ` Segher Boessenkool
2022-03-10 9:35 ` Jakub Jelinek
2022-03-10 20:36 ` Michael Meissner
0 siblings, 2 replies; 13+ messages in thread
From: Segher Boessenkool @ 2022-03-09 22:57 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Wed, Mar 09, 2022 at 10:10:07PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2022 at 02:57:20PM -0600, Segher Boessenkool wrote:
> > But __ibm128 should *always* be supported, so this is a hypothetical
> > problem.
>
> I bet that will require much more work. I think for the barely supported
> (or really unsupported?) case of old sysv IEEE quad
The "q" library routines are double-double. On RIOS2 (POWER2) there
were "quad" instructions that worked on a pair of FP regs, but that was
handled as a pair of FP regs, and since 2012 we do not support POWER2
anymore anyway.
I have no clue if and when the "q_" library routines are used. The do
take KFmode params (or TFmode if we use double-double preferably).
Or are you thinking of something else still?
> or for when long double
> is just DFmode we'd need some more (IFmode?) for __ibm128 and deal with
> everything it needs (mapping it to libgcc entrypoints etc.).
Well, the symbols would be called "tf", as all __ibm128 symbols are, for
backwards compatibility; all __ieee128 are "kf", no matter what. This
is much simpler than the mess we have with internal modes :-)
It also is no big deal if you get link errors for missing libraries if
you try to use an unsupported configuration. All the basic stuff will
still work. This is similar in ways to what happens if you try to use
too old system libraries, or a too old binutils.
> > If you are fed up with all this, please commit what you have now (after
> > testing of course ;-) ), and I'll pick up things myself. Either way,
> > thank you for all your work on this!
>
> Ok, here is what I'll test momentarily:
Thanks again!
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v3: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 22:57 ` Segher Boessenkool
@ 2022-03-10 9:35 ` Jakub Jelinek
2022-03-10 10:37 ` Segher Boessenkool
2022-03-10 20:36 ` Michael Meissner
1 sibling, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2022-03-10 9:35 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
On Wed, Mar 09, 2022 at 04:57:01PM -0600, Segher Boessenkool wrote:
> > > If you are fed up with all this, please commit what you have now (after
> > > testing of course ;-) ), and I'll pick up things myself. Either way,
> > > thank you for all your work on this!
> >
> > Ok, here is what I'll test momentarily:
>
> Thanks again!
Unfortunately, while regtesting on powerpc64le-linux went fine
(except for
l += 2.0;
in the last testcase should have been
h += 2.0;
already fixed in my copy), on powerpc64-linux it regressed (both -m32 and
-m64) following testcase:
+FAIL: gcc.target/powerpc/convert-fp-128.c (test for excess errors)
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendddtd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendddtf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extenddfdd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extenddftd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsddd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsddf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsdtd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsdtf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsfdd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsfsd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendsftd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_extendtftd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_truncdddf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_truncddsd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_truncddsf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_truncdfsd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_truncsdsf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctddd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctddf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctdsd2\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctdsf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctdtf\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctfdd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl __dpd_trunctfsd\\\\M 1
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mbl\\\\M 24
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mfmr\\\\M 0
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mfrsp|xsrsp\\\\M 2
+UNRESOLVED: gcc.target/powerpc/convert-fp-128.c scan-assembler-times \\\\mlfd\\\\M 2
The problem is that previously on the pre-VSX -mcpu=
where we support only TFmode being double double we accepted both:
typedef float __attribute__((mode(IF))) mode_if;
typedef float __attribute__((mode(KF))) mode_kf;
in the testcase. handle_mode_attribute calls
/* Allow the target a chance to translate MODE into something supported.
See PR86324. */
mode = targetm.translate_mode_attribute (mode);
and the rs6000 hook for it looks like:
/* Target hook for translate_mode_attribute. */
static machine_mode
rs6000_translate_mode_attribute (machine_mode mode)
{
if ((FLOAT128_IEEE_P (mode)
&& ieee128_float_type_node == long_double_type_node)
|| (FLOAT128_IBM_P (mode)
&& ibm128_float_type_node == long_double_type_node))
return COMPLEX_MODE_P (mode) ? E_TCmode : E_TFmode;
return mode;
}
With the v3 patch, ibm128_float_type_node == long_double_type_node
in that case and IF -> TF translation looks correct to me, under
the hood it will do the same thing.
But the fact that it accepted KFmode before and silently handled
it like TFmode, where KFmode should be IEEE quad, while TFmode in this
case is double double looks like a bug to me.
So, I think we just need to adjust the testcase.
As mode_kf is only used #ifdef __FLOAT128_TYPE__, I've guarded its
definition with that condition too.
Thus, here is what I've committed in the end.
Note, really unsure about backports, this patch is quite large and
changes behavior here and there. Probably easiest would be just
to revert those __SIZEOF_*128__ rs6000 change on release branches?
Or backport a strictly __SIZEOF_*128__ related change (such as
use TARGET_FLOAT128_TYPE as the condition on whether to predefine
those macros or not together with moving __SIZEOF_FLOAT128__ to the other
function next to __float128)?
2022-03-10 Jakub Jelinek <jakub@redhat.com>
PR target/99708
* config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Remove
RS6000_BTI_ptr_ieee128_float and RS6000_BTI_ptr_ibm128_float.
(ptr_ieee128_float_type_node, ptr_ibm128_float_type_node): Remove.
* config/rs6000/rs6000-builtin.cc (rs6000_type_string): Return
"**NULL**" if type_node is NULL first. Handle
ieee128_float_type_node.
(rs6000_init_builtins): Don't initialize ptr_ieee128_float_type_node
and ptr_ibm128_float_type_node. Set ibm128_float_type_node and
ieee128_float_type_node to NULL rather than long_double_type_node if
they aren't supported. Do support __ibm128 even if
!TARGET_FLOAT128_TYPE when long double is double double.
(rs6000_expand_builtin): Error if bif_is_ibm128 and
!ibm128_float_type_node. Remap RS6000_BIF_{,UN}PACK_IF to
RS6000_BIF_{,UN}PACK_TF much earlier and only use bif_is_ibm128 check
for it.
* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
__SIZEOF_FLOAT128__ here and only iff __float128 macro is defined.
(rs6000_cpu_cpp_builtins): Don't define __SIZEOF_FLOAT128__ here.
Define __SIZEOF_IBM128__=16 if ieee128_float_type_node is non-NULL.
Formatting fix.
* config/rs6000/rs6000-gen-builtins.cc: Document ibm128 attribute.
(struct attrinfo): Add isibm128 member.
(TYPE_MAP_SIZE): Remove.
(type_map): Use [] instead of [TYPE_MAP_SIZE]. For "if" use
ibm128_float_type_node only if it is non-NULL, otherwise fall back
to long_double_type_node. Remove "pif" entry.
(parse_bif_attrs): Handle ibm128 attribute and print it for debugging.
(write_decls): Output bif_ibm128_bit and bif_is_ibm128.
(write_type_node): Use sizeof type_map / sizeof type_map[0]
instead of TYPE_MAP_SIZE.
(write_bif_static_init): Handle isibm128.
* config/rs6000/rs6000-builtins.def: Document ibm128 attribute.
(__builtin_pack_ibm128, __builtin_unpack_ibm128): Add ibm128
attribute.
* gcc.dg/pr99708.c: New test.
* gcc.target/powerpc/pr99708-2.c: New test.
* gcc.target/powerpc/convert-fp-128.c (mode_kf): Define only if
__FLOAT128_TYPE__ is defined.
--- gcc/config/rs6000/rs6000.h.jj 2022-03-09 15:24:50.647017881 +0100
+++ gcc/config/rs6000/rs6000.h 2022-03-09 19:55:31.879255798 +0100
@@ -2444,8 +2444,6 @@ enum rs6000_builtin_type_index
RS6000_BTI_ptr_long_double,
RS6000_BTI_ptr_dfloat64,
RS6000_BTI_ptr_dfloat128,
- RS6000_BTI_ptr_ieee128_float,
- RS6000_BTI_ptr_ibm128_float,
RS6000_BTI_ptr_vector_pair,
RS6000_BTI_ptr_vector_quad,
RS6000_BTI_ptr_long_long,
@@ -2541,8 +2539,6 @@ enum rs6000_builtin_type_index
#define ptr_long_double_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_double])
#define ptr_dfloat64_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat64])
#define ptr_dfloat128_type_node (rs6000_builtin_types[RS6000_BTI_ptr_dfloat128])
-#define ptr_ieee128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ieee128_float])
-#define ptr_ibm128_float_type_node (rs6000_builtin_types[RS6000_BTI_ptr_ibm128_float])
#define ptr_vector_pair_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_pair])
#define ptr_vector_quad_type_node (rs6000_builtin_types[RS6000_BTI_ptr_vector_quad])
#define ptr_long_long_integer_type_node (rs6000_builtin_types[RS6000_BTI_ptr_long_long])
--- gcc/config/rs6000/rs6000-builtin.cc.jj 2022-03-09 15:24:50.642017950 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc 2022-03-09 20:10:53.778612345 +0100
@@ -402,7 +402,9 @@ rs6000_vector_type (const char *name, tr
static
const char *rs6000_type_string (tree type_node)
{
- if (type_node == void_type_node)
+ if (type_node == NULL_TREE)
+ return "**NULL**";
+ else if (type_node == void_type_node)
return "void";
else if (type_node == long_integer_type_node)
return "long";
@@ -432,6 +434,8 @@ const char *rs6000_type_string (tree typ
return "ss";
else if (type_node == ibm128_float_type_node)
return "__ibm128";
+ else if (type_node == ieee128_float_type_node)
+ return "__ieee128";
else if (type_node == opaque_V4SI_type_node)
return "opaque";
else if (POINTER_TYPE_P (type_node))
@@ -709,9 +713,9 @@ rs6000_init_builtins (void)
For IEEE 128-bit floating point, always create the type __ieee128. If the
user used -mfloat128, rs6000-c.cc will create a define from __float128 to
__ieee128. */
- if (TARGET_FLOAT128_TYPE)
+ if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE))
{
- if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
+ if (!TARGET_IEEEQUAD)
ibm128_float_type_node = long_double_type_node;
else
{
@@ -721,22 +725,24 @@ rs6000_init_builtins (void)
layout_type (ibm128_float_type_node);
}
t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
- ptr_ibm128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ibm128_float_type_node,
"__ibm128");
+ }
+ else
+ ibm128_float_type_node = NULL_TREE;
+ if (TARGET_FLOAT128_TYPE)
+ {
if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
ieee128_float_type_node = long_double_type_node;
else
ieee128_float_type_node = float128_type_node;
t = build_qualified_type (ieee128_float_type_node, TYPE_QUAL_CONST);
- ptr_ieee128_float_type_node = build_pointer_type (t);
lang_hooks.types.register_builtin_type (ieee128_float_type_node,
"__ieee128");
}
-
else
- ieee128_float_type_node = ibm128_float_type_node = long_double_type_node;
+ ieee128_float_type_node = NULL_TREE;
/* Vector pair and vector quad support. */
vector_pair_type_node = make_node (OPAQUE_TYPE);
@@ -3418,6 +3424,13 @@ rs6000_expand_builtin (tree exp, rtx tar
return const0_rtx;
}
+ if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node)
+ {
+ error ("%qs requires %<__ibm128%> type support",
+ bifaddr->bifname);
+ return const0_rtx;
+ }
+
if (bif_is_cpu (*bifaddr))
return cpu_expand_builtin (fcode, exp, target);
@@ -3498,6 +3511,21 @@ rs6000_expand_builtin (tree exp, rtx tar
gcc_unreachable ();
}
+ if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
+ {
+ if (fcode == RS6000_BIF_PACK_IF)
+ {
+ icode = CODE_FOR_packtf;
+ fcode = RS6000_BIF_PACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ else if (fcode == RS6000_BIF_UNPACK_IF)
+ {
+ icode = CODE_FOR_unpacktf;
+ fcode = RS6000_BIF_UNPACK_TF;
+ uns_fcode = (size_t) fcode;
+ }
+ }
/* TRUE iff the built-in function returns void. */
bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node;
@@ -3642,23 +3670,6 @@ rs6000_expand_builtin (tree exp, rtx tar
if (bif_is_mma (*bifaddr))
return mma_expand_builtin (exp, target, icode, fcode);
- if (fcode == RS6000_BIF_PACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_packtf;
- fcode = RS6000_BIF_PACK_TF;
- uns_fcode = (size_t) fcode;
- }
- else if (fcode == RS6000_BIF_UNPACK_IF
- && TARGET_LONG_DOUBLE_128
- && !TARGET_IEEEQUAD)
- {
- icode = CODE_FOR_unpacktf;
- fcode = RS6000_BIF_UNPACK_TF;
- uns_fcode = (size_t) fcode;
- }
-
if (TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node)
target = NULL_RTX;
else if (target == 0
--- gcc/config/rs6000/rs6000-c.cc.jj 2022-03-09 15:24:50.643017937 +0100
+++ gcc/config/rs6000/rs6000-c.cc 2022-03-09 19:55:31.880255784 +0100
@@ -584,6 +584,10 @@ rs6000_target_modify_macros (bool define
rs6000_define_or_undefine_macro (true, "__float128=__ieee128");
else
rs6000_define_or_undefine_macro (false, "__float128");
+ if (ieee128_float_type_node && define_p)
+ rs6000_define_or_undefine_macro (true, "__SIZEOF_FLOAT128__=16");
+ else
+ rs6000_define_or_undefine_macro (false, "__SIZEOF_FLOAT128__");
}
/* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
via the target attribute/pragma. */
@@ -623,11 +627,11 @@ rs6000_cpu_cpp_builtins (cpp_reader *pfi
if (TARGET_FRSQRTES)
builtin_define ("__RSQRTEF__");
if (TARGET_FLOAT128_TYPE)
- builtin_define ("__FLOAT128_TYPE__");
+ builtin_define ("__FLOAT128_TYPE__");
if (ibm128_float_type_node)
builtin_define ("__SIZEOF_IBM128__=16");
if (ieee128_float_type_node)
- builtin_define ("__SIZEOF_FLOAT128__=16");
+ builtin_define ("__SIZEOF_IEEE128__=16");
#ifdef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
builtin_define ("__BUILTIN_CPU_SUPPORTS__");
#endif
--- gcc/config/rs6000/rs6000-gen-builtins.cc.jj 2022-03-09 15:24:50.644017923 +0100
+++ gcc/config/rs6000/rs6000-gen-builtins.cc 2022-03-09 19:55:31.880255784 +0100
@@ -93,6 +93,8 @@ along with GCC; see the file COPYING3.
lxvrze Needs special handling for load-rightmost, zero-extended
endian Needs special handling for endianness
ibmld Restrict usage to the case when TFmode is IBM-128
+ ibm128 Restrict usage to the case where __ibm128 is supported or
+ if ibmld
An example stanza might look like this:
@@ -392,6 +394,7 @@ struct attrinfo
bool islxvrze;
bool isendian;
bool isibmld;
+ bool isibm128;
};
/* Fields associated with a function prototype (bif or overload). */
@@ -492,8 +495,7 @@ struct typemap
maps tokens from a fntype string to a tree type. For example,
in "si_ftype_hi" we would map "si" to "intSI_type_node" and
map "hi" to "intHI_type_node". */
-#define TYPE_MAP_SIZE 86
-static typemap type_map[TYPE_MAP_SIZE] =
+static typemap type_map[] =
{
{ "bi", "bool_int" },
{ "bv16qi", "bool_V16QI" },
@@ -506,7 +508,9 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "df", "double" },
{ "di", "long_long_integer" },
{ "hi", "intHI" },
- { "if", "ibm128_float" },
+ { "if", "ibm128_float_type_node "
+ "? ibm128_float_type_node "
+ ": long_double" },
{ "ld", "long_double" },
{ "lg", "long_integer" },
{ "pbv16qi", "ptr_bool_V16QI" },
@@ -519,7 +523,6 @@ static typemap type_map[TYPE_MAP_SIZE] =
{ "pdf", "ptr_double" },
{ "pdi", "ptr_long_long_integer" },
{ "phi", "ptr_intHI" },
- { "pif", "ptr_ibm128_float" },
{ "pld", "ptr_long_double" },
{ "plg", "ptr_long_integer" },
{ "pqi", "ptr_intQI" },
@@ -1439,6 +1442,8 @@ parse_bif_attrs (attrinfo *attrptr)
attrptr->isendian = 1;
else if (!strcmp (attrname, "ibmld"))
attrptr->isibmld = 1;
+ else if (!strcmp (attrname, "ibm128"))
+ attrptr->isibm128 = 1;
else
{
diag (oldpos, "unknown attribute.\n");
@@ -1472,14 +1477,15 @@ parse_bif_attrs (attrinfo *attrptr)
"ldvec = %d, stvec = %d, reve = %d, pred = %d, htm = %d, "
"htmspr = %d, htmcr = %d, mma = %d, quad = %d, pair = %d, "
"mmaint = %d, no32bit = %d, 32bit = %d, cpu = %d, ldstmask = %d, "
- "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld= %d.\n",
+ "lxvrse = %d, lxvrze = %d, endian = %d, ibmdld = %d, ibm128 = %d.\n",
attrptr->isinit, attrptr->isset, attrptr->isextract,
attrptr->isnosoft, attrptr->isldvec, attrptr->isstvec,
attrptr->isreve, attrptr->ispred, attrptr->ishtm, attrptr->ishtmspr,
attrptr->ishtmcr, attrptr->ismma, attrptr->isquad, attrptr->ispair,
attrptr->ismmaint, attrptr->isno32bit, attrptr->is32bit,
attrptr->iscpu, attrptr->isldstmask, attrptr->islxvrse,
- attrptr->islxvrze, attrptr->isendian, attrptr->isibmld);
+ attrptr->islxvrze, attrptr->isendian, attrptr->isibmld,
+ attrptr->isibm128);
#endif
return PC_OK;
@@ -2294,6 +2300,7 @@ write_decls (void)
fprintf (header_file, "#define bif_lxvrze_bit\t\t(0x00100000)\n");
fprintf (header_file, "#define bif_endian_bit\t\t(0x00200000)\n");
fprintf (header_file, "#define bif_ibmld_bit\t\t(0x00400000)\n");
+ fprintf (header_file, "#define bif_ibm128_bit\t\t(0x00800000)\n");
fprintf (header_file, "\n");
fprintf (header_file,
"#define bif_is_init(x)\t\t((x).bifattrs & bif_init_bit)\n");
@@ -2341,6 +2348,8 @@ write_decls (void)
"#define bif_is_endian(x)\t((x).bifattrs & bif_endian_bit)\n");
fprintf (header_file,
"#define bif_is_ibmld(x)\t((x).bifattrs & bif_ibmld_bit)\n");
+ fprintf (header_file,
+ "#define bif_is_ibm128(x)\t((x).bifattrs & bif_ibm128_bit)\n");
fprintf (header_file, "\n");
fprintf (header_file,
@@ -2385,8 +2394,10 @@ write_type_node (char *tok, bool indent)
{
if (indent)
fprintf (init_file, " ");
- typemap *entry = (typemap *) bsearch (tok, type_map, TYPE_MAP_SIZE,
- sizeof (typemap), typemap_cmp);
+ typemap *entry
+ = (typemap *) bsearch (tok, type_map,
+ sizeof type_map / sizeof type_map[0],
+ sizeof (typemap), typemap_cmp);
if (!entry)
fatal ("Type map is inconsistent.");
fprintf (init_file, "%s_type_node", entry->value);
@@ -2535,6 +2546,8 @@ write_bif_static_init (void)
fprintf (init_file, " | bif_endian_bit");
if (bifp->attrs.isibmld)
fprintf (init_file, " | bif_ibmld_bit");
+ if (bifp->attrs.isibm128)
+ fprintf (init_file, " | bif_ibm128_bit");
fprintf (init_file, ",\n");
fprintf (init_file, " /* restr_opnd */\t{%d, %d, %d},\n",
bifp->proto.restr_opnd[0], bifp->proto.restr_opnd[1],
--- gcc/config/rs6000/rs6000-builtins.def.jj 2022-03-09 15:24:50.643017937 +0100
+++ gcc/config/rs6000/rs6000-builtins.def 2022-03-09 19:55:31.880255784 +0100
@@ -138,6 +138,7 @@
; lxvrze Needs special handling for load-rightmost, zero-extended
; endian Needs special handling for endianness
; ibmld Restrict usage to the case when TFmode is IBM-128
+; ibm128 Restrict usage to the case where __ibm128 is supported or if ibmld
;
; Each attribute corresponds to extra processing required when
; the built-in is expanded. All such special processing should
@@ -234,13 +235,13 @@
MTFSF rs6000_mtfsf {}
const __ibm128 __builtin_pack_ibm128 (double, double);
- PACK_IF packif {}
+ PACK_IF packif {ibm128}
void __builtin_set_fpscr_rn (const int[0,3]);
SET_FPSCR_RN rs6000_set_fpscr_rn {}
const double __builtin_unpack_ibm128 (__ibm128, const int<1>);
- UNPACK_IF unpackif {}
+ UNPACK_IF unpackif {ibm128}
; This is redundant with __builtin_unpack_ibm128, as it requires long
; double to be __ibm128. Should probably be deprecated.
--- gcc/testsuite/gcc.dg/pr99708.c.jj 2022-03-09 19:55:31.880255784 +0100
+++ gcc/testsuite/gcc.dg/pr99708.c 2022-03-09 19:55:31.880255784 +0100
@@ -0,0 +1,7 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_FLOAT128__
+__float128 f = 1.0;
+#endif
+long double l = 1.0;
--- gcc/testsuite/gcc.target/powerpc/pr99708-2.c.jj 2022-03-09 19:55:31.881255770 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr99708-2.c 2022-03-09 19:55:31.880255784 +0100
@@ -0,0 +1,22 @@
+/* PR target/99708 */
+/* { dg-do compile } */
+
+#ifdef __SIZEOF_IBM128__
+__ibm128 f = 1.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+__ieee128 g = 1.0;
+#endif
+long double h = 1.0;
+
+void
+foo (void)
+{
+#ifdef __SIZEOF_IBM128__
+ f += 2.0;
+#endif
+#ifdef __SIZEOF_IEEE128__
+ g += 2.0;
+#endif
+ h += 2.0;
+}
--- gcc/testsuite/gcc.target/powerpc/convert-fp-128.c.jj 2020-07-28 15:39:10.048755678 +0200
+++ gcc/testsuite/gcc.target/powerpc/convert-fp-128.c 2022-03-10 10:14:50.681047411 +0100
@@ -7,7 +7,9 @@
#define mode_sf float
#define mode_df double
typedef float __attribute__((mode(IF))) mode_if;
+#ifdef __FLOAT128_TYPE__
typedef float __attribute__((mode(KF))) mode_kf;
+#endif
#define mode_sd _Decimal32
#define mode_dd _Decimal64
#define mode_td _Decimal128
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v3: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-10 9:35 ` Jakub Jelinek
@ 2022-03-10 10:37 ` Segher Boessenkool
0 siblings, 0 replies; 13+ messages in thread
From: Segher Boessenkool @ 2022-03-10 10:37 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Michael Meissner, Jonathan Wakely, gcc-patches, David Edelsohn
Hi!
On Thu, Mar 10, 2022 at 10:35:36AM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2022 at 04:57:01PM -0600, Segher Boessenkool wrote:
> > > > If you are fed up with all this, please commit what you have now (after
> > > > testing of course ;-) ), and I'll pick up things myself. Either way,
> > > > thank you for all your work on this!
> > >
> > > Ok, here is what I'll test momentarily:
> >
> > Thanks again!
>
> Unfortunately, while regtesting on powerpc64le-linux went fine
> (except for
> l += 2.0;
> in the last testcase should have been
> h += 2.0;
> already fixed in my copy), on powerpc64-linux it regressed (both -m32 and
> -m64) following testcase:
>
> +FAIL: gcc.target/powerpc/convert-fp-128.c (test for excess errors)
<snip>
> The problem is that previously on the pre-VSX -mcpu=
> where we support only TFmode being double double we accepted both:
> typedef float __attribute__((mode(IF))) mode_if;
> typedef float __attribute__((mode(KF))) mode_kf;
There is no KFmode in that case, so the test case is just broken? (It
should not depend on VSX at all, but that has been the situation since
forever).
It is not hard to ICE the compiler with bad mode attributes, this has
nothing to do with IEEE QP or anything. It is comparable to how with
bad inline assembler you can cause ICEs (by giving RA no way out, that
it can see anyway).
> in the testcase. handle_mode_attribute calls
> /* Allow the target a chance to translate MODE into something supported.
> See PR86324. */
> mode = targetm.translate_mode_attribute (mode);
> and the rs6000 hook for it looks like:
> /* Target hook for translate_mode_attribute. */
> static machine_mode
> rs6000_translate_mode_attribute (machine_mode mode)
> {
> if ((FLOAT128_IEEE_P (mode)
> && ieee128_float_type_node == long_double_type_node)
> || (FLOAT128_IBM_P (mode)
> && ibm128_float_type_node == long_double_type_node))
> return COMPLEX_MODE_P (mode) ? E_TCmode : E_TFmode;
> return mode;
> }
Bah. That looks like a workaround for some other bug :-(
> With the v3 patch, ibm128_float_type_node == long_double_type_node
> in that case and IF -> TF translation looks correct to me, under
> the hood it will do the same thing.
We should not use IFmode at all there, because it will *not* do the same
thing: we do not handle IFmode in all cases there, only the few that use
this hook.
This needs to be fixed. That needs fixes in generic code, that still
thinks there is a total order on the floating point modes; but there are
IFmode values that are not representable in KFmode, and KFmode values
that are not representable in IFmode. Pretending they can be ordered
(in any direction) gives problems. This is why we have these
workarounds.
> But the fact that it accepted KFmode before and silently handled
> it like TFmode, where KFmode should be IEEE quad, while TFmode in this
> case is double double looks like a bug to me.
Yes. That is exactly why I did not want KFmode to be handled as the
wrong (but valid) mode: it hides problems, and not in a harmless way,
it makes problems much harder to find.
> So, I think we just need to adjust the testcase.
Hopefully! There may be other things still depending on this
translation, so this does not give me the warm fuzzies :-(
> As mode_kf is only used #ifdef __FLOAT128_TYPE__, I've guarded its
> definition with that condition too.
>
> Thus, here is what I've committed in the end.
>
> Note, really unsure about backports, this patch is quite large and
> changes behavior here and there. Probably easiest would be just
> to revert those __SIZEOF_*128__ rs6000 change on release branches?
Yes.
> Or backport a strictly __SIZEOF_*128__ related change (such as
> use TARGET_FLOAT128_TYPE as the condition on whether to predefine
> those macros or not together with moving __SIZEOF_FLOAT128__ to the other
> function next to __float128)?
And then more projects would need extra checks for broken code, making
this whole __SIZEOF_* thing less useful? Not a fan. Also, this would
be not a real backport at all :-(
Thanks again,
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000, v3: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]
2022-03-09 22:57 ` Segher Boessenkool
2022-03-10 9:35 ` Jakub Jelinek
@ 2022-03-10 20:36 ` Michael Meissner
1 sibling, 0 replies; 13+ messages in thread
From: Michael Meissner @ 2022-03-10 20:36 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Jakub Jelinek, Michael Meissner, Jonathan Wakely, gcc-patches,
David Edelsohn
On Wed, Mar 09, 2022 at 04:57:01PM -0600, Segher Boessenkool wrote:
> On Wed, Mar 09, 2022 at 10:10:07PM +0100, Jakub Jelinek wrote:
> > On Wed, Mar 09, 2022 at 02:57:20PM -0600, Segher Boessenkool wrote:
> > > But __ibm128 should *always* be supported, so this is a hypothetical
> > > problem.
> >
> > I bet that will require much more work. I think for the barely supported
> > (or really unsupported?) case of old sysv IEEE quad
>
> The "q" library routines are double-double. On RIOS2 (POWER2) there
> were "quad" instructions that worked on a pair of FP regs, but that was
> handled as a pair of FP regs, and since 2012 we do not support POWER2
> anymore anyway.
>
> I have no clue if and when the "q_" library routines are used. The do
> take KFmode params (or TFmode if we use double-double preferably).
>
> Or are you thinking of something else still?
Probably libquadmath (which we still build). Libquadmath defines all of the
functions with a 'q' suffix.
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ defines [PR99708]
2022-03-07 21:37 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT, IBM}128__ " Segher Boessenkool
2022-03-09 13:27 ` [PATCH] rs6000, v2: " Jakub Jelinek
@ 2022-03-10 20:44 ` Michael Meissner
1 sibling, 0 replies; 13+ messages in thread
From: Michael Meissner @ 2022-03-10 20:44 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Jakub Jelinek, David Edelsohn, gcc-patches, Michael Meissner,
Jonathan Wakely
On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> Hi!
>
> On Sat, Mar 05, 2022 at 09:21:51AM +0100, Jakub Jelinek wrote:
> > As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
> > macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
> > is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
> > actually supported or not.
> >
> > The following patch:
> > 1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
> > really support them instead of equal to long_double_type_node
> > 2) adjusts the builtins code to use
> > ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > for the 2 builtins, so that we don't ICE during builtins creation
> > if ibm128_float_type_node is NULL (using error_mark_node instead of
> > long_double_type_node sounds more risky to me)
>
> I feel the opposite way: (potentially) using the wrong thing is just a
> ticking time bomb, never "safer".
>
> > 3) in rs6000_type_string will not match NULL as __ibm128, and adds
> > a __ieee128 case
> > 4) removes the clearly unused ptr_{ieee,ibm}128_float_type_node trees;
> > if something needs them in the future, they can be easily added back,
> > but wasting GTY just in case...
> > 5) actually syncs __SIZEOF_FLOAT128__ with the __float128 macro being
> > defined in addition to __ieee128 being usable
> >
> > Now, in the PR Segher said he doesn't like 5), but I think it is better
> > to match the reality and get this PR fixed and if we want to rethink
> > how __float128 is defined (whether it is a macro, or perhaps another
> > builtin type like __ieee128 which could be easily done by
> > lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> > "__ieee128");
> > lang_hooks.types.register_builtin_type (ieee128_float_type_node,
> > "__float128");
> > perhaps under some conditions, rethink if the -mfloat128 option exists
> > and what it does etc., it can be done incrementally and the rs6000-c.cc
> > hunks in the patch can be easily reverted (appart from the formatting
> > fix).
>
> There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
> Sorry I did not pick up on that earlier.
No, no, no.
The '__ieee128' keyword was used as a way to define the type but not enable the
'__float128' keyword. Then rs6000-c.cc defines __float128 to be __ieee128,
similar to defining 'vector' and '__vector' to be
__attribute__((altivec(vector__))'.
Unfortunately, there is no way to remove a keyword after the creation (or at
least there wasn't in the GCC 8 time frame when I wrote the code), and you need
to create the types at GCC startup to set up the built-ins.
No one should be using '__ieee128'. The official keywords are '__float128' and
for C (not C++) '_Float128'.
Perhaps in GCC 13 it is time to just remove it and always just define
'__float128' instead.
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-03-10 20:44 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-05 8:21 [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ defines [PR99708] Jakub Jelinek
2022-03-07 21:37 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT, IBM}128__ " Segher Boessenkool
2022-03-09 13:27 ` [PATCH] rs6000, v2: " Jakub Jelinek
2022-03-09 14:00 ` Jonathan Wakely
2022-03-09 18:34 ` Segher Boessenkool
2022-03-09 19:24 ` Jakub Jelinek
2022-03-09 20:57 ` Segher Boessenkool
2022-03-09 21:10 ` [PATCH] rs6000, v3: " Jakub Jelinek
2022-03-09 22:57 ` Segher Boessenkool
2022-03-10 9:35 ` Jakub Jelinek
2022-03-10 10:37 ` Segher Boessenkool
2022-03-10 20:36 ` Michael Meissner
2022-03-10 20:44 ` [PATCH] rs6000: Fix up __SIZEOF_{FLOAT,IBM}128__ " Michael Meissner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).