Hi!
On Thu, Oct 22, 2020 at 06:03:46PM -0400, Michael Meissner wrote:
> To map the scanf functions, <name> is mapped to __isoc99_<name>ieee128.
Is that correct? What if you are compiling for c90?
> * 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.
"Map the built-in function names" etc.
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -26893,56 +26893,127 @@ 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. */
I don't see why that is the right thing to do? You'll have exactly
these same problems on 32-bit!
Hrm, we talked about that before I guess? Do you just need to change
this comment now?
> + default:
> + break;
> + }
That is useless, just leave it out? The end of a switch will always
fall through, and it is normal idiom to use that.
> + /* Update the __builtin_*printf && __builtin_*scanf functions. */
"and" :-)
> + else if (name[len - 1] == 'l')
> + {
> + bool uses_ieee128_p = false;
> + tree type = TREE_TYPE (decl);
> + machine_mode ret_mode = TYPE_MODE (type);
> +
> + /* 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
> {
> - machine_mode arg_mode = TYPE_MODE (arg);
> - if (arg_mode == TFmode || arg_mode == TCmode)
> + function_args_iterator args_iter;
> + tree arg;
(declare that right before the FOREACH)
> +
> + /* 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;
> + }
> }
> }
There is no point in doing all these early-outs in an initialisation
function, making it much harder to read :-(
> + /* If we passed or returned an IEEE 128-bit floating point type,
> + change the name. Use __<name>ieee128, instead of <name>l. */
> + if (uses_ieee128_p)
> + newname = xasprintf ("__%.*sieee128", (int)(len - 1), name);
(int) (len - 1)
Please comment what the - 1 does, and/or what this is for at all. (In
the code / in a comment, not to me, I figured it out after a while.)
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-longdouble-math.c
> @@ -0,0 +1,567 @@
> +/* { dg-require-effective-target ppc_float128_hw } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power9 -mno-pcrel -O2 -Wno-psabi
> -mabi=ieeelongdouble" } */
So why do you need power10_ok? You need power9_ok of course, but why
power10? You can specify -mno-pcrel always.
> + /* { dg-final { scan-assembler {\mxsrqpi +[0-9]+,[0-9]+,[0-9]+,2\M} } } */
The " +" should be just a " ".
Not every assembler variant uses plain numbers for registers. That is
not currently an issue of course, but why not just do it right in the
first place :-)
You can do
{\mxsrqpi r?[0-9]+,r?[0-9]+,r?[0-9]+,2\M}
or simpler,
{\mxsrqpi r?\d+,r?\d+,r?\d+,2\M}
or even simpler
{(?n)\mxsrqpi .*,2\M}
(if you don't mind the number of operands in that last case).
> + /* { dg-final { scan-assembler {\mxsrqpi +[0-9]+,[0-9]+,[0-9]+,3\M} } } */
(same as above)
> + /* lgammaf128 mentioned previously. */
> + *p++ = BUILTIN1 (lgammal, *q++);
What does that comment mean? Should you have used scan-assembler-times
perhaps?
> + /* { dg-final { scan-assembler {\mxsrqpi +[0-9]+,[0-9]+,[0-9]+,1\M} } } */
(one more)
> + /* remainderf128 mentioned previously. */
> + *p++ = BUILTIN2 (remainderl, *q++, *r++);
(similar to the lgammal case)
> + /* { dg-final { scan-assembler {\m__scalbnieee128\M} } } */
> + *p = BUILTIN2 (scalbl, *q, *r);
(trailing spaces)
> + /* { dg-final { scan-assembler {\m__cabsieee128\M} } } */
> + *p++ = BUILTIN1 (cabsl, *q++);
> +
> + /* inline code. */
> + *p++ = BUILTIN1 (cargl, *q++);
> +
> + /* inline code. */
> + *p++ = BUILTIN1 (cimagl, *q++);
> +
> + /* inline code. */
> + *p = BUILTIN1 (creall, *q);
(trailing spaces)
So no checking here at all? Aww :-)
> + /* inline code. */
> + *p++ = BUILTIN1 (conjl, *q++);
(another)
> + /* __lgammaieee128_r mentioned previously. */
> + *p = BUILTIN2 (lgammal_r, *q, *r);
(-times again; and trailing spaces)
> + /* { dg-final { scan-assembler {\m__scalbnieee128\M} } } */
> + *p = BUILTIN2 (scalbnl, *q, *r);
(guess)
(Many more later. I don't care in a testcase, but just make your editor
not make you do this? :-) )
> +/* Debug main program to determine if the library has all of the mapped
> + external functions. Note, you need a new enough glibc to provide all of
> the
> + f128 functions. */
> +#ifdef DO_MAIN
> +int
> +main (void)
> +{
> + return 0;
> +}
> +#endif
You don't need that #ifdef/#endif, that will always just compile? Oh,
and you don't mention this is to check if linking works, with a manual
command line, aha.
Do you want to make this a link test then? I guess it is hard to detect
whether you have a new enough glibc though :-/
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-longdouble-stdio.c
> @@ -0,0 +1,37 @@
> +/* { dg-require-effective-target ppc_float128_hw } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power9 -mno-pcrel -O2 -Wno-psabi
> -mabi=ieeelongdouble" } */
Same power10_ok issue as the other test.
Segher
