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, BUILT_IN_NORMAL))
--
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
