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