| Message ID | 20181023201211.GA5010@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | , PowerPC: Use <math>f128 for long double built-ins if we have changed to use IEEE 128-bit floating point | expand |
Hi Mike, On Tue, Oct 23, 2018 at 04:12:11PM -0400, Michael Meissner wrote: > This patch changes the name used by the <math>l built-in functions that return > or are passed long double if the long double type is changed from the current > IBM long double format to the IEEE 128-bit format. > > I have done a bootstrap and make check with no regressions on a little endian > power8 system. Is it ok to check into the trunk? This will need to be back > ported to the GCC 8.x branch. Could you test it on the usual assortment of systems instead of just one? BE 7 and 8, LE 8 and 9. Or wait for possible fallout :-) A backport to 8 is wanted yes, but please wait as usual. It's fine after a week or so of no problems. > +/* On 64-bit Linux and Freebsd systems, possibly switch the long double library > + function names from <foo>l to <foo>f128 if the default long double type is > + IEEE 128-bit. Typically, with the C and C++ languages, the standard math.h > + include file switches the names on systems that support long double as IEEE > + 128-bit, but that doesn't work if the user uses __builtin_<foo>l directly or > + if they use Fortran. Use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to > + change this name. We only do this if the default is long double is not IEEE > + 128-bit, and the user asked for IEEE 128-bit. */ s/default is/default/ Does this need some synchronisation with the libm headers? I guess things will just work out, but it is desirable if libm stops doing this with compilers that have this change? > +static tree > +rs6000_mangle_decl_assembler_name (tree decl, tree id) > +{ > + if (!TARGET_IEEEQUAD_DEFAULT && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128 Write this is in the opposite order? if (TARGET_LONG_DOUBLE_128 && TARGET_IEEEQUAD && !TARGET_IEEEQUAD_DEFAULT > + { > + size_t len = IDENTIFIER_LENGTH (id); > + const char *name = IDENTIFIER_POINTER (id); > + > + if (name[len-1] == 'l') > + { > + bool has_long_double_p = false; > + tree type = TREE_TYPE (decl); > + machine_mode ret_mode = TYPE_MODE (type); > + > + /* See if the function returns long double or long double > + complex. */ > + if (ret_mode == TFmode || ret_mode == TCmode) > + has_long_double_p = true; This comment is a bit misleading I think? The code checks if it is the same mode as would be used for long double, not if that is the actual asked-for type. The code is fine AFAICS, the comment isn't so great though. > + else > + { > + function_args_iterator args_iter; > + tree arg; > + > + /* See if we have a long double or long double complex > + argument. */ And same here. > + FOREACH_FUNCTION_ARGS (type, arg, args_iter) > + { > + machine_mode arg_mode = TYPE_MODE (arg); > + if (arg_mode == TFmode || arg_mode == TCmode) > + { > + has_long_double_p = true; > + break; > + } > + } > + } > + > + /* If we have long double, change the name. */ And this. > + if (has_long_double_p) > + { > + char *name2 = (char *) alloca (len + 4); > + memcpy (name2, name, len-1); len - 1 Okay for trunk with those things fixed. Thanks! Segher
On Tue, Oct 23, 2018 at 04:18:55PM -0500, Segher Boessenkool wrote: > Hi Mike, > > On Tue, Oct 23, 2018 at 04:12:11PM -0400, Michael Meissner wrote: > > This patch changes the name used by the <math>l built-in functions that return > > or are passed long double if the long double type is changed from the current > > IBM long double format to the IEEE 128-bit format. > > > > I have done a bootstrap and make check with no regressions on a little endian > > power8 system. Is it ok to check into the trunk? This will need to be back > > ported to the GCC 8.x branch. > > Could you test it on the usual assortment of systems instead of just one? > BE 7 and 8, LE 8 and 9. Or wait for possible fallout :-) Ok. > A backport to 8 is wanted yes, but please wait as usual. It's fine after > a week or so of no problems. Yep. > > > +/* On 64-bit Linux and Freebsd systems, possibly switch the long double library > > + function names from <foo>l to <foo>f128 if the default long double type is > > + IEEE 128-bit. Typically, with the C and C++ languages, the standard math.h > > + include file switches the names on systems that support long double as IEEE > > + 128-bit, but that doesn't work if the user uses __builtin_<foo>l directly or > > + if they use Fortran. Use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to > > + change this name. We only do this if the default is long double is not IEEE > > + 128-bit, and the user asked for IEEE 128-bit. */ > > s/default is/default/ > > Does this need some synchronisation with the libm headers? I guess things > will just work out, but it is desirable if libm stops doing this with > compilers that have this change? It should just work out assuming you are using a recent enough GLIBC. The patch is more for when you aren't using headers. > > +static tree > > +rs6000_mangle_decl_assembler_name (tree decl, tree id) > > +{ > > + if (!TARGET_IEEEQUAD_DEFAULT && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128 > > Write this is in the opposite order? > if (TARGET_LONG_DOUBLE_128 && TARGET_IEEEQUAD && !TARGET_IEEEQUAD_DEFAULT Because !TARGET_IEEEQUAD_DEFAULT is a constant test. If you are on a system that defaults to IEEE 128-bit, the whole code gets deleted. I would hope the tests still get deleted if it occurs later in the test, but I tend to put the things that can be optimized at compile time first. > > + { > > + size_t len = IDENTIFIER_LENGTH (id); > > + const char *name = IDENTIFIER_POINTER (id); > > + > > + if (name[len-1] == 'l') > > + { > > + bool has_long_double_p = false; > > + tree type = TREE_TYPE (decl); > > + machine_mode ret_mode = TYPE_MODE (type); > > + > > + /* See if the function returns long double or long double > > + complex. */ > > + if (ret_mode == TFmode || ret_mode == TCmode) > > + has_long_double_p = true; > > This comment is a bit misleading I think? The code checks if it is the > same mode as would be used for long double, not if that is the actual > asked-for type. The code is fine AFAICS, the comment isn't so great > though. Well the long double type is 'TFmode'. Though _Float128 does get mapped to TFmode instead of KFmode also. But explicit f128 built-ins won't go through here, since they don't end in 'l'. I'm just trying to avoid things like CLZL that take long arguments and not long double. > > + else > > + { > > + function_args_iterator args_iter; > > + tree arg; > > + > > + /* See if we have a long double or long double complex > > + argument. */ > > And same here. > > > + FOREACH_FUNCTION_ARGS (type, arg, args_iter) > > + { > > + machine_mode arg_mode = TYPE_MODE (arg); > > + if (arg_mode == TFmode || arg_mode == TCmode) > > + { > > + has_long_double_p = true; > > + break; > > + } > > + } > > + } > > + > > + /* If we have long double, change the name. */ > > And this. > > > + if (has_long_double_p) > > + { > > + char *name2 = (char *) alloca (len + 4); > > + memcpy (name2, name, len-1); > > len - 1 Ok. > Okay for trunk with those things fixed. Thanks! > > > Segher >
On Tue, 23 Oct 2018, Michael Meissner wrote: > 2018-10-23 Michael Meissner <meissner@linux.ibm.com> > > * config/rs6000/rs6000.c (TARGET_MANGLE_DECL_ASSEMBLER_NAME): > Define as rs6000_mangle_decl_assembler_name. > (rs6000_mangle_decl_assembler_name): If the user switched from IBM > long double to IEEE long double, switch the names of the long > double built-in functions to be <func>f128 instead of <func>l. [This is the issue discussed at the Cauldron of how to get the redirections correct in any case that does not involve including the standard <math.h> to get the function declarations.] My understanding was that __<func>ieee128 aliases would be added to glibc (indeed, the relevant names are present in sysdeps/ieee754/ldbl-128ibm-compat/Versions in glibc, albeit without actually being enabled for powerpc64le yet), for two reasons: (a) to be namespace-clean for standard C, and (b) because a few libm functions (and many libc functions) have no _Float128 analogues but are still part of the API for long double (e.g. the nexttoward functions, where nexttowardf, nexttoward also need different versions for IEEE 128-bit long double, or scalbl, which is somewhat obsolescent but still supported). Now, you can't use the __<func>ieee128 names with *current* glibc because they aren't exported yet. So is the plan that GCC would later switch to using the __<func>ieee128 names when available based on TARGET_GLIBC_MAJOR and TARGET_GLIBC_MINOR (as more namespace-correct, and available for some functions without <func>f128 variants), with use of <func>f128 only a fallback for older glibc versions lacking __<func>ieee128?
On Tue, Oct 23, 2018 at 10:22:41PM +0000, Joseph Myers wrote: > On Tue, 23 Oct 2018, Michael Meissner wrote: > > > 2018-10-23 Michael Meissner <meissner@linux.ibm.com> > > > > * config/rs6000/rs6000.c (TARGET_MANGLE_DECL_ASSEMBLER_NAME): > > Define as rs6000_mangle_decl_assembler_name. > > (rs6000_mangle_decl_assembler_name): If the user switched from IBM > > long double to IEEE long double, switch the names of the long > > double built-in functions to be <func>f128 instead of <func>l. > > [This is the issue discussed at the Cauldron of how to get the > redirections correct in any case that does not involve including the > standard <math.h> to get the function declarations.] > > My understanding was that __<func>ieee128 aliases would be added to glibc > (indeed, the relevant names are present in > sysdeps/ieee754/ldbl-128ibm-compat/Versions in glibc, albeit without > actually being enabled for powerpc64le yet), for two reasons: (a) to be > namespace-clean for standard C, and (b) because a few libm functions (and > many libc functions) have no _Float128 analogues but are still part of the > API for long double (e.g. the nexttoward functions, where nexttowardf, > nexttoward also need different versions for IEEE 128-bit long double, or > scalbl, which is somewhat obsolescent but still supported). > > Now, you can't use the __<func>ieee128 names with *current* glibc because > they aren't exported yet. So is the plan that GCC would later switch to > using the __<func>ieee128 names when available based on TARGET_GLIBC_MAJOR > and TARGET_GLIBC_MINOR (as more namespace-correct, and available for some > functions without <func>f128 variants), with use of <func>f128 only a > fallback for older glibc versions lacking __<func>ieee128? I suspect the timing may not be right for GCC 9, since I tend to use the Advance Toolchain to get newer glibc's, and they are on 2.28. I just tried it, and as you said, they aren't exported yet. However, I am somewhat relunctant to do patches if I don't have a glibc that properly exports them.
Here is the patch I just committed. [gcc] 2018-10-24 Michael Meissner <meissner@linux.ibm.com> * config/rs6000/rs6000.c (TARGET_MANGLE_DECL_ASSEMBLER_NAME): Define as rs6000_mangle_decl_assembler_name. (rs6000_mangle_decl_assembler_name): If the user switched from IBM long double to IEEE long double, switch the names of the long double built-in functions to be <func>f128 instead of <func>l. [gcc/testsuite] 2018-10-24 Michael Meissner <meissner@linux.ibm.com> * gcc.target/powerpc/float128-math.c: New test to make sure the long double built-in function names use the f128 form if the user switched from IBM long double to IEEE long double. * gcc.target/powerpc/ppc-fortran/ieee128-math.f90: Likewise.
On Tue, Oct 23, 2018 at 05:53:15PM -0400, Michael Meissner wrote: > > > +static tree > > > +rs6000_mangle_decl_assembler_name (tree decl, tree id) > > > +{ > > > + if (!TARGET_IEEEQUAD_DEFAULT && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128 > > > > Write this is in the opposite order? > > if (TARGET_LONG_DOUBLE_128 && TARGET_IEEEQUAD && !TARGET_IEEEQUAD_DEFAULT > > Because !TARGET_IEEEQUAD_DEFAULT is a constant test. If you are on a system > that defaults to IEEE 128-bit, the whole code gets deleted. I would hope the > tests still get deleted if it occurs later in the test, but I tend to put the > things that can be optimized at compile time first. The compiler can work out what is constant by itself just fine ;-) Please try to optimise our code for readability, instead. TARGET_IEEEQUAD_DEFAULT does not make much sense unless TARGET_IEEEQUAD is set, and that itself not unless TARGET_LONG_DOUBLE is set. > > This comment is a bit misleading I think? The code checks if it is the > > same mode as would be used for long double, not if that is the actual > > asked-for type. The code is fine AFAICS, the comment isn't so great > > though. > > Well the long double type is 'TFmode'. Though _Float128 does get mapped to > TFmode instead of KFmode also. But explicit f128 built-ins won't go through > here, since they don't end in 'l'. I'm just trying to avoid things like CLZL > that take long arguments and not long double. modes and types are very different things. A mode is something on the level of your machine language, while a type is something on the level of your source language. "long double" is a type, and it can be DFmode, TFmode, IFmode, KFmode. IFmode can also be __ibm128, etc. Talking about "types" here is very confusing. Segher
On Tue, Oct 23, 2018 at 07:40:04PM -0400, Michael Meissner wrote: > On Tue, Oct 23, 2018 at 10:22:41PM +0000, Joseph Myers wrote: > > Now, you can't use the __<func>ieee128 names with *current* glibc because > > they aren't exported yet. So is the plan that GCC would later switch to > > using the __<func>ieee128 names when available based on TARGET_GLIBC_MAJOR > > and TARGET_GLIBC_MINOR (as more namespace-correct, and available for some > > functions without <func>f128 variants), with use of <func>f128 only a > > fallback for older glibc versions lacking __<func>ieee128? > > I suspect the timing may not be right for GCC 9, since I tend to use the > Advance Toolchain to get newer glibc's, and they are on 2.28. I just tried it, > and as you said, they aren't exported yet. GCC 9 won't be released until half a year from now. Segher
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 265400) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -1981,6 +1981,9 @@ static const struct attribute_spec rs600 #undef TARGET_SETJMP_PRESERVES_NONVOLATILE_REGS_P #define TARGET_SETJMP_PRESERVES_NONVOLATILE_REGS_P hook_bool_void_true + +#undef TARGET_MANGLE_DECL_ASSEMBLER_NAME +#define TARGET_MANGLE_DECL_ASSEMBLER_NAME rs6000_mangle_decl_assembler_name /* Processor table. */ @@ -38965,6 +38968,67 @@ rs6000_globalize_decl_name (FILE * strea #endif +/* On 64-bit Linux and Freebsd systems, possibly switch the long double library + function names from <foo>l to <foo>f128 if the default long double type is + IEEE 128-bit. Typically, with the C and C++ languages, the standard math.h + include file switches the names on systems that support long double as IEEE + 128-bit, but that doesn't work if the user uses __builtin_<foo>l directly or + if they use Fortran. Use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to + change this name. We only do this if the default is long double is not IEEE + 128-bit, and the user asked for IEEE 128-bit. */ + +static tree +rs6000_mangle_decl_assembler_name (tree decl, tree id) +{ + if (!TARGET_IEEEQUAD_DEFAULT && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128 + && TREE_CODE (decl) == FUNCTION_DECL && DECL_IS_BUILTIN (decl) ) + { + size_t len = IDENTIFIER_LENGTH (id); + const char *name = IDENTIFIER_POINTER (id); + + if (name[len-1] == 'l') + { + bool has_long_double_p = false; + tree type = TREE_TYPE (decl); + machine_mode ret_mode = TYPE_MODE (type); + + /* See if the function returns long double or long double + complex. */ + if (ret_mode == TFmode || ret_mode == TCmode) + has_long_double_p = true; + else + { + function_args_iterator args_iter; + tree arg; + + /* See if we have a long double or long double complex + argument. */ + FOREACH_FUNCTION_ARGS (type, arg, args_iter) + { + machine_mode arg_mode = TYPE_MODE (arg); + if (arg_mode == TFmode || arg_mode == TCmode) + { + has_long_double_p = true; + break; + } + } + } + + /* If we have long double, change the name. */ + if (has_long_double_p) + { + char *name2 = (char *) alloca (len + 4); + memcpy (name2, name, len-1); + strcpy (name2 + len - 1, "f128"); + id = get_identifier (name2); + } + } + } + + return id; +} + + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-rs6000.h" Index: gcc/testsuite/gcc.target/powerpc/float128-math.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/float128-math.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/float128-math.c (working copy) @@ -0,0 +1,20 @@ +/* { dg-do compile { target { powerpc*-*-linux* } } } */ +/* { dg-require-effective-target ppc_float128_sw } */ +/* { dg-require-effective-target vsx_hw } */ +/* { dg-options "-mvsx -O2 -mfloat128 -mabi=ieeelongdouble -Wno-psabi" } */ + +/* Test whether we convert __builtin_<math>l to __builtin_<math>f128 if the + default long double type is IEEE 128-bit. Also test that using the explicit + __builtin_<math>f128 function does not interfere with the __builtin_<math>l + function. */ + +extern __float128 sinf128 (__float128); + +void foo (__float128 *p, long double *q, long double *r) +{ + *p = sinf128 (*p); + *q = __builtin_sinl (*q); +} + +/* { dg-final { scan-assembler-times {\mbl sinf128\M} 2 } } */ +/* { dg-final { scan-assembler-not {\mbl sinl\M} } } */ Index: gcc/testsuite/gcc.target/powerpc/ppc-fortran/ieee128-math.f90 =================================================================== --- gcc/testsuite/gcc.target/powerpc/ppc-fortran/ieee128-math.f90 (revision 0) +++ gcc/testsuite/gcc.target/powerpc/ppc-fortran/ieee128-math.f90 (working copy) @@ -0,0 +1,20 @@ +! { dg-do compile { target { powerpc*-*-linux* } } } +! { dg-require-effective-target ppc_float128_sw } +! { dg-require-effective-target vsx_hw } +! { dg-options "-mvsx -mabi=ieeelongdouble -mfloat128" } +! { dg-excess-errors "expect error due to switching long double type" } +! Since the error message is not associated with a particular line +! number, we cannot use the dg-error directive and cannot specify a +! regexp to describe the expected error message. The expected warning +! message is: +! "Warning: Using IEEE extended precision long double [-Wpsabi]" + +program test_qp + implicit none + real(16), volatile :: fp1, fp2; + fp1 = 2.0 + fp2 = log (fp1) +end + +! { dg-final { scan-assembler-not {\mbl logl\M} } } +! { dg-final { scan-assembler {\mbl logf128\M} } }