* [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).