Re: [PATCH 1/9, revised] PowerPC: Map long double built-in functions if IEEE 128-bit long double.
On Fri, Oct 16, 2020 at 03:30:56PM -0500, Segher Boessenkool wrote: > > + if (len >= printf_len > > + && strcmp (name + len - printf_len, "printf") == 0) > > Thew first test is unnecessary. Actually no, it is necessary. If you are looking at a builtin function with 4 or fewer characters, if you don't check if len is at least 5 (i.e. >= printf_len), the expression: name + len - printf_len would be before the beginning of the name pointer. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
Re: [PATCH 1/9, revised] PowerPC: Map long double built-in functions if IEEE 128-bit long double.
On Fri, Oct 16, 2020 at 03:30:56PM -0500, Segher Boessenkool wrote:
> On Fri, Oct 09, 2020 at 12:35:44AM -0400, Michael Meissner wrote:
> > This patch is revised from the first version of the patch posted.
>
> In the future, please send a NEW series, in a NEW thread, when you have
> a new series. I was waiting for a new series (because you needed
> changes), and I missed that you posted it in the old thread.
>
> If you do not want to because the patches are independent (and not
> actually a series), you can send them as independent patches as well
> (and that is easier to handle, independent things become much more
> obviously independent!)
In this case, they are grouped together because various people (Tulio, Jonathan
Wakely) need to know all of the patches that they need to apply to get their
various work (GLIBC, Libstc++, etc.) to work before the patches are installed.
> > Normally the mapping is done in the math.h and stdio.h files. However, not
> > everybody uses these files, which means we also need to change the external
> > name for the built-in function within the compiler.
>
> Because those are *not* defined in those headers. So those headers are
> completely irrelevant to this.
The current glibc stdio.h and math.h takes care of the mapping for the
functions without having the compiler do the mapping. But not everybody uses
those headers (there are a few test cases for instance that do not include
math.h).
But more importantly, we need the mapping for Fortran to access the right
functions for the math library, since they do not have an equivalent of math.h.
>
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -26897,56 +26897,156 @@ rs6000_globalize_decl_name (FILE * stream, tree
> > decl)
> > library before you can switch the real*16 type at compile time.
> >
> > We use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change this name.
> > We
> > - only do this if the default is that long double is IBM extended double,
> > and
> > - the user asked for IEEE 128-bit. */
> > + only do this transformation if the __float128 type is enabled. This
> > + prevents us from doing the transformation on older 32-bit ports that
> > might
> > + have enabled using IEEE 128-bit floating point as the default long
> > double
> > + type. */
>
> But that is just as broken then?
I'm not sure what you mean is broken. We currently do not provide the float128
emulation functions on any system other than little endian PowerPC. If
somebody goes through and adds the work, then obviously, we can change the
code.
>
> > + default:
> > + break;
>
> Please don't invert the natural order, leave the default at the bottom.
> Just like you should not write "0 == x".
Ok.
>
> > + case BUILT_IN_NEXTTOWARD:
> > + newname = "__nexttoward_to_ieee128";
> > + break;
> > +
> > + case BUILT_IN_NEXTTOWARDF:
> > + newname = "__nexttowardf_to_ieee128";
> > + break;
>
> Why the "_to_" in those? How irregular.
No idea. I was just using the names that are in glibc.
> > + case BUILT_IN_SCALBL:
> > + newname = "__scalbnieee128";
> > + break;
>
> Should that be __scalbieee128?
Yes, thanks.
> > + /* Update the __builtin_*printf && __builtin_*scanf functions. */
> > + if (!newname)
> > + {
> > + const size_t printf_len = sizeof ("printf") - 1;
>
> The "const" here has no function; the compiler *knows* this is a
> constant number.
>
> Please use strlen, and no - 1.
Ok.
> > + if (len >= printf_len
> > + && strcmp (name + len - printf_len, "printf") == 0)
>
> Thew first test is unnecessary.
>
> > + {
> > + char *name2 = (char *) alloca (len + 1 + printf_extra);
> > + strcpy (name2, "__");
> > + memcpy (name2 + 2, name, len);
> > + strcpy (name2 + 2 + len, "ieee128");
> > + newname = (const char *) name2;
> > + }
>
> Maybe just use asprintf and simplify all of this massively? It's not
> like a malloc here or there will be measurably slower (if it was, you
> need to do all of this differently anyway, with some caching etc.)
Ok.
> > + /* See if the function passes a IEEE 128-bit floating point
> > type
> > +or complex type. */
> > + FOREACH_FUNCTION_ARGS (type, arg, args_iter)
> > {
> > - uses_ieee128_p = true;
> > - break;
> > + machine_mode arg_mode = TYPE_MODE (arg);
> > + if (arg_mode == TFmode || arg_mode == TCmode)
> > + {
> > + uses_ieee128_p = true;
> > + break;
> > + }
>
> I don't see why TFmode would mean it is IEEE? TFmode can be IBM128 as
> well?
The whole section is inside of the test:
if (TARGET_FLOAT128_TYPE && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128
&& TREE_CODE (decl) == FUNCTION_DECL
&& fndecl_built_in_p (decl, BUI
Re: [PATCH 1/9, revised] PowerPC: Map long double built-in functions if IEEE 128-bit long double.
On Fri, Oct 09, 2020 at 12:35:44AM -0400, Michael Meissner wrote:
> This patch is revised from the first version of the patch posted.
In the future, please send a NEW series, in a NEW thread, when you have
a new series. I was waiting for a new series (because you needed
changes), and I missed that you posted it in the old thread.
If you do not want to because the patches are independent (and not
actually a series), you can send them as independent patches as well
(and that is easier to handle, independent things become much more
obviously independent!)
> Normally the mapping is done in the math.h and stdio.h files. However, not
> everybody uses these files, which means we also need to change the external
> name for the built-in function within the compiler.
Because those are *not* defined in those headers. So those headers are
completely irrelevant to this.
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -26897,56 +26897,156 @@ rs6000_globalize_decl_name (FILE * stream, tree
> decl)
> library before you can switch the real*16 type at compile time.
>
> We use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change this name. We
> - only do this if the default is that long double is IBM extended double,
> and
> - the user asked for IEEE 128-bit. */
> + only do this transformation if the __float128 type is enabled. This
> + prevents us from doing the transformation on older 32-bit ports that might
> + have enabled using IEEE 128-bit floating point as the default long double
> + type. */
But that is just as broken then?
> + default:
> + break;
Please don't invert the natural order, leave the default at the bottom.
Just like you should not write "0 == x".
> + case BUILT_IN_NEXTTOWARD:
> + newname = "__nexttoward_to_ieee128";
> + break;
> +
> + case BUILT_IN_NEXTTOWARDF:
> + newname = "__nexttowardf_to_ieee128";
> + break;
Why the "_to_" in those? How irregular.
> + case BUILT_IN_SCALBL:
> + newname = "__scalbnieee128";
> + break;
Should that be __scalbieee128?
> + /* Update the __builtin_*printf && __builtin_*scanf functions. */
> + if (!newname)
> + {
> + const size_t printf_len = sizeof ("printf") - 1;
The "const" here has no function; the compiler *knows* this is a
constant number.
Please use strlen, and no - 1.
> + if (len >= printf_len
> + && strcmp (name + len - printf_len, "printf") == 0)
Thew first test is unnecessary.
> + {
> + char *name2 = (char *) alloca (len + 1 + printf_extra);
> + strcpy (name2, "__");
> + memcpy (name2 + 2, name, len);
> + strcpy (name2 + 2 + len, "ieee128");
> + newname = (const char *) name2;
> + }
Maybe just use asprintf and simplify all of this massively? It's not
like a malloc here or there will be measurably slower (if it was, you
need to do all of this differently anyway, with some caching etc.)
> + /* See if the function passes a IEEE 128-bit floating point
> type
> + or complex type. */
> + FOREACH_FUNCTION_ARGS (type, arg, args_iter)
> {
> - uses_ieee128_p = true;
> - break;
> + machine_mode arg_mode = TYPE_MODE (arg);
> + if (arg_mode == TFmode || arg_mode == TCmode)
> + {
> + uses_ieee128_p = true;
> + break;
> + }
I don't see why TFmode would mean it is IEEE? TFmode can be IBM128 as
well?
Segher
[PATCH 1/9, revised] PowerPC: Map long double built-in functions if IEEE 128-bit long double.
PowerPC: Map long double built-in functions if IEEE 128-bit long double.
This patch is revised from the first version of the patch posted. It uses the
names that are not in the user's namespace (i.e. __sinieee128 instead of
sinf128) that Joseph Myers suggested.
This patch goes through the built-in functions and changes the name of the
math, scanf, and printf built-in functions to use the functions that GLIBC
provides when long double uses the IEEE 128-bit representation.
Normally the mapping is done in the math.h and stdio.h files. However, not
everybody uses these files, which means we also need to change the external
name for the built-in function within the compiler.
In addition, changing the name in GCC allows the Fortran compiler to
automatically use the correct name.
To map the math functions, typically this patch changes l to
__ieee128. However there are some exceptions that are handled with this
patch.
To map the printf functions, is mapped to __ieee128.
To map the scanf functions, is mapped to __isoc99ieee128.
gcc/
2020-10-09 Michael Meissner
* config/rs6000/rs6000.c (rs6000_mangle_decl_assembler_name): Add
support for mapping built-in function names for long double
built-in functions if long double is IEEE 128-bit.
gcc/testsuite/
2020-10-09 Michael Meissner
* gcc.target/powerpc/float128-longdouble-math.c: New test.
* gcc.target/powerpc/float128-longdouble-stdio.c: New test.
* gcc.target/powerpc/float128-math.c: Adjust test for new name
being generated.
---
gcc/config/rs6000/rs6000.c| 162 -
.../powerpc/float128-longdouble-math.c| 567 ++
.../powerpc/float128-longdouble-stdio.c | 37 ++
.../gcc.target/powerpc/float128-math.c| 6 +-
4 files changed, 738 insertions(+), 34 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/float128-longdouble-math.c
create mode 100644 gcc/testsuite/gcc.target/powerpc/float128-longdouble-stdio.c
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index b28f4adf464..4c141c9f276 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -26897,56 +26897,156 @@ rs6000_globalize_decl_name (FILE * stream, tree decl)
library before you can switch the real*16 type at compile time.
We use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change this name. We
- only do this if the default is that long double is IBM extended double, and
- the user asked for IEEE 128-bit. */
+ only do this transformation if the __float128 type is enabled. This
+ prevents us from doing the transformation on older 32-bit ports that might
+ have enabled using IEEE 128-bit floating point as the default long double
+ type. */
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) )
+ if (TARGET_FLOAT128_TYPE && TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128
+ && TREE_CODE (decl) == FUNCTION_DECL
+ && fndecl_built_in_p (decl, BUILT_IN_NORMAL))
{
size_t len = IDENTIFIER_LENGTH (id);
const char *name = IDENTIFIER_POINTER (id);
+ const char *newname = NULL;
- if (name[len - 1] == 'l')
+ /* See if it is one of the built-in functions with an unusual name. */
+ switch (DECL_FUNCTION_CODE (decl))
{
- bool uses_ieee128_p = false;
- tree type = TREE_TYPE (decl);
- machine_mode ret_mode = TYPE_MODE (type);
+ default:
+ break;
- /* See if the function returns a IEEE 128-bit floating point type or
-complex type. */
- if (ret_mode == TFmode || ret_mode == TCmode)
- uses_ieee128_p = true;
- else
+ case BUILT_IN_DREML:
+ newname = "__remainderieee128";
+ break;
+
+ case BUILT_IN_GAMMAL:
+ newname = "__lgammaieee128";
+ break;
+
+ case BUILT_IN_GAMMAL_R:
+ case BUILT_IN_LGAMMAL_R:
+ newname = "__lgammaieee128_r";
+ break;
+
+ case BUILT_IN_NEXTTOWARD:
+ newname = "__nexttoward_to_ieee128";
+ break;
+
+ case BUILT_IN_NEXTTOWARDF:
+ newname = "__nexttowardf_to_ieee128";
+ break;
+
+ case BUILT_IN_NEXTTOWARDL:
+ newname = "__nexttowardieee128";
+ break;
+
+ case BUILT_IN_POW10L:
+ newname = "__exp10ieee128";
+ break;
+
+ case BUILT_IN_SCALBL:
+ newname = "__scalbnieee128";
+ break;
+
+ case BUILT_IN_SIGNIFICANDL:
+ newname = "__significandieee128";
+ break;
+
+ case BUILT_IN_SINCOSL:
+ newname = "__sincosieee128";
+ break;
+ }
+
+ /* Update the __builtin_*printf && __builtin_*scanf functions. */
+ if (!newname)
+ {
+ const size_t printf_len = sizeof ("
