Bug 25477 - builtin functions should use $LDBL128 suffix on darwin when appropriate
Summary: builtin functions should use $LDBL128 suffix on darwin when appropriate
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: 4.4.0
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: patch, wrong-code
: 23504 25850 (view as bug list)
Depends on: 19984
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-18 05:42 UTC by Andrew Pinski
Modified: 2008-02-28 14:54 UTC (History)
4 users (show)

See Also:
Host:
Target: powerpc-*-darwin[789]*
Build:
Known to work: 4.4.0
Known to fail: 4.2.0 4.3.0
Last reconfirmed: 2008-02-23 11:08:16


Attachments
Partial patch (959 bytes, patch)
2008-02-22 00:34 UTC, Francois-Xavier Coudert
Details | Diff
Complete patch (1.40 KB, patch)
2008-02-22 10:21 UTC, Francois-Xavier Coudert
Details | Diff
Addendum to the ldouble patch (571 bytes, patch)
2008-02-25 11:29 UTC, Uroš Bizjak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Pinski 2005-12-18 05:42:25 UTC
The following program does not work if we don't include math.h and stdio.h.

long double f(long double a)
{
  return __builtin_sqrtl (a);
}

int main(void)
{
  __builtin_printf("%Lf %f\n", f(2.0) + 2, 200.0);
}

The fortran failure gfortran.dg/large_real_kind_2.F90 is the same issue.

Note __builtin_printf is wrong even when including stdio.h
Comment 1 Andrew Pinski 2005-12-18 05:46:24 UTC
Here is what we currently get:
_f:
        b L_sqrtl$stub

.....
        b L_printf$stub

-------
When including math.h:
_f:
        b L_sqrtl$LDBL128$stub



I am going to try to fix this.

More info can be found at:
http://gcc.gnu.org/ml/gcc/2005-12/msg00505.html
Comment 2 Andrew Pinski 2005-12-18 06:06:07 UTC
(In reply to comment #1)
> Here is what we currently get:

Oh, I Know why printf is broken, it is broken because I am still using 10.3.9 which does not have a fixed up stdio.h.
Comment 3 Andrew Pinski 2006-02-20 23:53:48 UTC
No longer working on this, too much troubles are causing to me to fix Darwin bugs.
Comment 4 Andrew Pinski 2006-03-02 00:42:08 UTC
Mine, this is will be one of my last Darwin specific patches.  Hopefully PR 23504 gets resolved with it too.
Comment 5 Andrew Pinski 2006-05-21 20:27:24 UTC
I am no longer working on this one.
Comment 6 Geoff Keating 2006-10-06 19:14:21 UTC
*** Bug 25850 has been marked as a duplicate of this bug. ***
Comment 7 Geoff Keating 2006-10-06 19:16:10 UTC
*** Bug 23504 has been marked as a duplicate of this bug. ***
Comment 8 Geoff Keating 2006-10-06 19:20:11 UTC
The general problem here is that on PPC Darwin, when using gcc with -mlong-double-128 (which is the default), some system functions (you can find a list in bug 25850) need to have $LDBL128 appended to their assembly names.  For the variadic functions, like printf, which might not have any long doubles passed to them, this should happen only when compiling for 10.3.9 and later, since these functions aren't available earlier; the user needs to avoid calling those functions with long doubles when compiling with a -mmacosx-version-min of less than 10.3.9.
Comment 9 Jack Howarth 2006-10-06 22:46:40 UTC
Geoff,
    Are the variadic functions really worth worrying about? Currently gcc trunk can only
be built with the cctools from Xcode 2.4 or later (which basically limits the builds to
MacOS X 10.4 machines unless a odcctools build of cctools is used). So for stock
configurations, gcc trunk will be unusable on MacOS X 10.3.9. Perhaps we should 
declare gcc 4.2 as MacOS X 10.4 or later?
                   Jack
ps I am referring the requirement of the newer cctools for literal16.
Comment 10 Geoff Keating 2006-10-06 23:05:54 UTC
(In reply to comment #9)
> Geoff,
>     Are the variadic functions really worth worrying about? Currently gcc trunk
> can only
> be built with the cctools from Xcode 2.4 or later (which basically limits the
> builds to
> MacOS X 10.4 machines unless a odcctools build of cctools is used). So for
> stock
> configurations, gcc trunk will be unusable on MacOS X 10.3.9. Perhaps we should 
> declare gcc 4.2 as MacOS X 10.4 or later?
>                    Jack
> ps I am referring the requirement of the newer cctools for literal16.

You're confusing host and target.  cctools runs on the host, but printf$LDBL128 is a function that exists (or not) on the target.
Comment 11 Jack Howarth 2006-10-06 23:54:24 UTC
Geoff,
     Okay. Any chance you or Mike could take a stab at patching this bug in time for gcc 4.2?
               Jack
Comment 12 INCORRECT EMAIL ADDRESS! 2006-10-07 00:28:58 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix on darwin when appropriate

"howarth at nitro dot med dot uc dot edu" <gcc-bugzilla@gcc.gnu.org> writes:

> Any chance you or Mike could take a stab at patching this bug in
> time for gcc 4.2?

No chance.  It's not a regression and so not suitable for 4.2.  Even for 4.3,
I have no reason to think it's more important than what I'm spending
time on now.  Why don't you fix it, if you think it's important?
Comment 13 Francois-Xavier Coudert 2008-02-21 14:12:02 UTC
Still there with on trunk (4.4) and MacOS 10.5.2:

$ cat s.c
int main (void)
{
  long double x;
  double y;

  x = -0.24; y = x;
  __builtin_printf ("%g %g %g\n", (float) __builtin_asinl(x),
                    (float) __builtin_asin(y),
                    (float) (__builtin_asinl(x) - __builtin_asin(y)));
}

$ ./bin/gcc s.c && ./a.out
-0.242366 -0.242366 0.5

asin() and asinl() values are very close, but their difference is output as 0.5. Adding a simple #include <math.h> on top of this makes it pass:

$ cat s2.c
#include <math.h>
int main (void)
{
  long double x;
  double y;

  x = -0.24; y = x;
  __builtin_printf ("%g %g %g\n", (float) __builtin_asinl(x),
                    (float) __builtin_asin(y),
                    (float) (__builtin_asinl(x) - __builtin_asin(y)));
}
$ ./bin/gcc s2.c && ./a.out
-0.242366 -0.242366 -8.82009e-18
Comment 14 Richard Biener 2008-02-21 15:48:23 UTC
How does the header manage to fix this?
Comment 15 Andrew Pinski 2008-02-21 15:50:10 UTC
(In reply to comment #14)
> How does the header manage to fix this?

The normal functions have the asm extension on them which causes the builtins to change also.

-- Pinski
Comment 16 Paolo Bonzini 2008-02-21 16:32:49 UTC
So this line of add_builtin_function

      tree libname = get_identifier (library_name);

should instead invoke a target hook (defaulting to get_identifier)?
Comment 17 Uroš Bizjak 2008-02-21 21:28:07 UTC
(In reply to comment #16)
> So this line of add_builtin_function
> 
>       tree libname = get_identifier (library_name);
> 
> should instead invoke a target hook (defaulting to get_identifier)?

And IMO http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25850#c6 should be wired into this hook.
Comment 18 Francois-Xavier Coudert 2008-02-22 00:34:59 UTC
Created attachment 15203 [details]
Partial patch

Another approach, maybe more efficient, is to offer a target hook to "fix builtins" after they're defined. That's the approach Andrew favoured in PR 25850. Putting together Andrew's code, the list of functions to fix by Mike (where non-builtins have been removed, and variadic functions are fixed conditionaly on macos_version_min, as per Geoff's advice... we get the patch attached. The only thing remaining, as far as I can see, is to find where to call this single target hook: Paolo, Uros, could you work that bit out? (It's probably trivial for someone who knows the guts of the middle-end, but I don't.)

PS: I also need to find how to make that part of darwin.c compiled only for PowerPC. The "#if defined (TARGET_TOC)" trick is apparently used in other places in darwin.c, but I'm a bit reluctant. Isn't there a Better Way?
Comment 19 Jack Howarth 2008-02-22 04:08:51 UTC
FX,
     I posted a modified version of Andrew's proposed patch awhile back...

http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01809.html

which had additional changes to gcc/config/rs6000/darwin.h and 
gcc/config/rs6000/rs6000.c for calling SUBTARGET_INIT_BUILTINS()
if defined (which would only be for powerpc darwin). I don't believe
that patch ever worked for me though...

http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01811.html
http://gcc.gnu.org/ml/gcc-patches/2007-05/msg02144.html

Comment 20 Uroš Bizjak 2008-02-22 06:54:53 UTC
(In reply to comment #18)

> call this single target hook: Paolo, Uros, could you work that bit out? (It's

I'll look into it.
Comment 21 Uroš Bizjak 2008-02-22 08:23:46 UTC
Hm, perhaps we should just put something like:

  set_optab_libfunc (sin_optab, TFmode, "sinl$LDBL128");
  ...

into rs6000_init_libfuncs()? At least this is what other targets do.
Comment 22 Paolo Bonzini 2008-02-22 08:25:32 UTC
You can call it at the end of rs6000_init_builtins, something like

#ifdef SUBTARGET_INIT_BUILTINS
  SUBTARGET_INIT_BUILTINS;
#endif

and in config/rs6000/darwin.h

#define SUBTARGET_INIT_BUILTINS darwin_patch_builtins ()

You don't need #ifdefs in darwin.c, it's just going to be dead code on i386.
Comment 23 Paolo Bonzini 2008-02-22 08:39:01 UTC
No, the math functions optabs are only used to invoke define_insns/define_expands, not to create libcalls.  If the optab says there is no insn, the original CALL_EXPR is expanded which uses the builtin's ASSEMBLER_NAME.
Comment 24 Uroš Bizjak 2008-02-22 09:28:03 UTC
Hm, for some reason the obvious approach from c#16 doesn't work:

The patch:

--cut here--
Index: langhooks.c
===================================================================
--- langhooks.c (revision 132541)
+++ langhooks.c (working copy)
@@ -559,7 +559,18 @@
 
   if (library_name)
     {
-      tree libname = get_identifier (library_name);
+      tree libname;
+
+      printf ("library_name = %s\n", library_name);
+      if (function_code == BUILT_IN_SINL)
+       {
+         printf ("PATCHING\n");
+         library_name = "sinl$LDBL128";
+       }
+
+      libname = get_identifier (library_name);
+      debug_tree (libname);
+
       SET_DECL_ASSEMBLER_NAME (decl, libname);
     }
 
--cut here--

the testcase:

long double test(long double x)
{
        return sinl(x);
}

debug output:

library_name = sinl
PATCHING
 <identifier_node 0xb7c893a8 sinl$LDBL128>

But still:

test:
        pushl   %ebp
        movl    %esp, %ebp
        popl    %ebp
        jmp     sinl
Comment 25 Francois-Xavier Coudert 2008-02-22 10:21:48 UTC
Created attachment 15206 [details]
Complete patch

That patch builds fine on powerpc-apple-darwin9.2.0. The C compiler generated seems OK:

$ cat s.c
#include <stdio.h>
int main (void)
{
  long double x;
  double y;

  x = -0.24; y = x;
  printf ("%g %g %g\n", (float) __builtin_asinl(x), (float) __builtin_asin(y),
          (float) (__builtin_asinl(x) - __builtin_asin(y)));
}

$ ./bin/gcc s.c -mlong-double-64 && ./a.out
-0.242366 -0.242366 0
$ nm ./a.out | grep asinl
         U _asinl

$ ./bin/gcc s.c -mlong-double-128 && ./a.out
-0.242366 -0.242366 -8.82009e-18
$ nm ./a.out | grep asinl
         U _asinl$LDBL128

but the Fortran front-end apparently doesn't benefit from it:

$ cat s.f90
  real(16) :: x
  real(8) :: y

  x = -0.24
  y = x
  print *, asin(x), asin(y), asin(x) - asin(y)
  print *, real(asin(x)), real(asin(y)), real(asin(x) - asin(y))
end
$ ./bin/gfortran s.f90 && ./a.out
 -0.24236584551303838594503263503820989       -0.24236584551303839       0.50000000000000000000000000000000000      
 -0.24236585     -0.24236585      0.50000000    
$ nm a.out | grep asinl
         U _asinl


Another issue is about -mmacosx-version-min. I'm not sure what should happen, but I have the following:

$ ./bin/gcc s.c -mmacosx-version-min=10.2.8 && ./a.out
-0.242366 -0.242366 -8.82009e-18
$ nm a.out | grep asinl
         U _asinl$LDBL128

If I understand correctly, this should give _asinl and not the $LDBL128 variant.

Any help appreciated.
Comment 26 Francois-Xavier Coudert 2008-02-22 10:40:08 UTC
> but the Fortran front-end apparently doesn't benefit from it:

The weird thing is that we go through the code of the patch, so I'm not sure why it still fails.
Comment 27 Paolo Bonzini 2008-02-22 10:47:56 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate


> but the Fortran front-end apparently doesn't benefit from it:

 From what I understand, you have to rebuild libgfortran.  Did you do so?

> $ ./bin/gcc s.c -mmacosx-version-min=10.2.8 && ./a.out
> -0.242366 -0.242366 -8.82009e-18
> $ nm a.out | grep asinl
>          U _asinl$LDBL128
> 
> If I understand correctly, this should give _asinl and not the $LDBL128
> variant.

No, only variadic functions like fprintf should be affected by 
-mmacosx-version-min.

Paolo
Comment 28 Dominique d'Humieres 2008-02-22 10:51:12 UTC
First thanks for looking to the problem. 

Second, all the machinery for adding $LDBL128 is provided by math.h (or in fixincludes/tests/base/architecture/ppc/math.h). So the problem for languages accepting to include math.h can be solved by adding the include in the code. Indeed for these languages it would be nice to do the include silently, but it's not vital to use long double.

Where the problem becomes serious is for languages such as fortran which has no direct way to access math.h. So far all the attempts I have seen have failed to yield a satisfactory solution. So either it is not as trivial as said by the Apple's guys, or if it is really why don't they give the hint?

Comment 29 Francois-Xavier Coudert 2008-02-22 11:00:38 UTC
>> but the Fortran front-end apparently doesn't benefit from it:
> 
>  From what I understand, you have to rebuild libgfortran.  Did you do so?

It doesn't need to, the calls are emitted directly by the front-end (otherwise, we'd be OK, as the library is in C and we do include <math.h>).

(As a sidenote, there is one minor thing for which recompiling libgfortran is needed, and that's specific functions; I'm not using them here. Specific functions are Fortran wrappers to math builtins that are compiled into libgfortran and used when a Fortran intrinsic math function is passed as an argument to another function.)

> No, only variadic functions like fprintf should be affected by 
> -mmacosx-version-min.

Of course, you're right, I got confused. It works:

$ ./bin/gcc -c s.c -mmacosx-version-min=10.3.9 
$ nm s.o | grep printf
         U _printf$LDBL128
$ ./bin/gcc -c s.c -mmacosx-version-min=10.3.8 
$ nm s.o | grep printf
         U _printf
Comment 30 Paolo Bonzini 2008-02-22 12:05:59 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate

> (As a sidenote, there is one minor thing for which recompiling libgfortran is
> needed, and that's specific functions; I'm not using them here. Specific
> functions are Fortran wrappers to math builtins that are compiled into
> libgfortran and used when a Fortran intrinsic math function is passed as an
> argument to another function.)

Yes, that's what I was thinking of.

Paolo
Comment 31 Paolo Bonzini 2008-02-22 12:08:24 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate


> Where the problem becomes serious is for languages such as fortran which has no
> direct way to access math.h. So far all the attempts I have seen have failed to
> yield a satisfactory solution. So either it is not as trivial as said by the
> Apple's guys, or if it is really why don't they give the hint?

Lack of interest for the platform.  After all, PowerBooks have been 
discontinued for 3 years... :-)

Paolo
Comment 32 Dominique d'Humieres 2008-02-22 13:12:57 UTC
> Lack of interest for the platform.  After all, PowerBooks have been 
> discontinued for 3 years... :-)

This PR is more than two year old and is present on all the ppc machines (including G5).
Note also that Apple has fixed at least one bug with long double on ppc in Darwin9.

My point was that, if the problem has a trivial fix, as claimed by the Darwin maintainers, they should at least provide the hint to those trying to fix the problem (what they never did). If such a trivial fix does not exist, they should acknowledge it. 

From my failed attempts to find a solution, I  got the impression that the simplest solution could be to add the suffix when generating the assembly, but I never found the time to check it (other than doing it by hand, reported somewhere on the lists).

Comment 33 Francois-Xavier Coudert 2008-02-22 13:22:47 UTC
(In reply to comment #26)
>> but the Fortran front-end apparently doesn't benefit from it:
> 
> The weird thing is that we go through the code of the patch, so I'm not sure
> why it still fails.

Here is the fndecl that is given to build_call_array() when emitting code for the call to __builtin_asinl:
  * debug_tree (DECL_ASSEMBLER_NAME (fndecl)) gives "<identifier_node 0x40f36a80 asinl$LDBL128>"
  * debug_tree (addr) gives (where addr is equal to build_addr (fndecl, current_function_decl)):

 <addr_expr 0x40f4b140
    type <pointer_type 0x40f48540
        type <function_type 0x40f1ec40 type <real_type 0x40f13c40 real(kind=16)>
            SI
            size <integer_cst 0x40cbc510 constant invariant 32>
            unit size <integer_cst 0x40cbc180 constant invariant 4>
            align 32 symtab 0 alias set -1 canonical type 0x40f1ec40
            arg-types <tree_list 0x40f24780 value <real_type 0x40f13c40 real(kind=16)>
                chain <tree_list 0x40f087a0 value <void_type 0x40f139a0 void>>>
            pointer_to_this <pointer_type 0x40f48540>>
        unsigned SI size <integer_cst 0x40cbc510 32> unit size <integer_cst 0x40cbc180 4>
        align 32 symtab 0 alias set -1 canonical type 0x40f48540>
    readonly constant invariant
    arg 0 <function_decl 0x40f26100 __builtin_asinl type <function_type 0x40f1ec40>
        readonly addressable public external built-in SI file (null) line 0 col 0
        align 32 built-in BUILT_IN_NORMAL:BUILT_IN_ASINL
        (mem:SI (symbol_ref:SI ("asinl") [flags 0x41] <function_decl 0x40f26100 __builtin_asinl>) [0 S4 A8])
        chain <function_decl 0x40f26080 __builtin_acoshf type <function_type 0x40f1e9a0>
            readonly public external built-in SI file (null) line 0 col 0
            align 32 built-in BUILT_IN_NORMAL:BUILT_IN_ACOSHF
            (mem:SI (symbol_ref:SI ("acoshf") [flags 0x41] <function_decl 0x40f26080 __builtin_acoshf>) [0 S4 A8]) chain <function_decl 0x40f26000 __builtin_acosh>>>>

So, the assembler name is set up correctly, and I don't see anything wrong in how we handle things. Question is: why do we still end up calling asinl and not asinl$LDBL128? In other terms, why aren't we using the assembler name of the function decl? 

PS: a simple testcase such as

  real function foo(x)
    real(16) :: x
    foo = asin(x)
  end

generates assembly that has "bl _asinl" instead of the expected "bl _asinl$LDB128".
Comment 34 Uroš Bizjak 2008-02-22 13:41:20 UTC
Perhaps patch from comment #24 can give some clue, because with this patch I get:

long double test(long double x) { return sinl(x); }
long double test_(long double x) { return __builtin_sinl(x); }

test_:
        jmp     sinl$LDBL128
test:
        jmp     sinl
Comment 35 Uroš Bizjak 2008-02-22 13:49:22 UTC
Hm...

/* Worker for DEF_BUILTIN.
   Possibly define a builtin function with one or two names.
   Does not declare a non-__builtin_ function if flag_no_builtin, or if
   nonansi_p and flag_no_nonansi_builtin.  */

static void
def_builtin_1 (enum built_in_function fncode,
	       const char *name,
	       enum built_in_class fnclass,
	       tree fntype, tree libtype,
	       bool both_p, bool fallback_p, bool nonansi_p,
	       tree fnattrs, bool implicit_p)
Comment 36 Paolo Bonzini 2008-02-22 14:02:30 UTC
I think you should use set_user_assembler_name instead of SET_DECL_ASSEMBLER_NAME.
Comment 37 Paolo Bonzini 2008-02-22 14:03:10 UTC
(Not that I knew that this morning, of course).
Comment 38 Francois-Xavier Coudert 2008-02-22 14:11:34 UTC
(In reply to comment #34)
> long double test(long double x) { return sinl(x); }
> long double test_(long double x) { return __builtin_sinl(x); }
> 
> test_:
>         jmp     sinl$LDBL128
> test:
>         jmp     sinl

I get the same thing with my patch. Isn't that just because sinl, without prototype, is seen as an extern function? (I don't know much about this)

I get a warning, though:
t.c:1: warning: incompatible implicit declaration of built-in function 'sinl'

Anyway, Fortran uses the function decls for BUILT_IN_SINL, which is the builtin one, so it shouldn't matter, should it?


(In reply to comment #36)
> I think you should use set_user_assembler_name instead of
> SET_DECL_ASSEMBLER_NAME.

Nope. Doing this gives, for you C testcase with test and test_:

Undefined symbols:
  "sinl$LDBL128", referenced from:
      _test_ in ccYtxtFH.o
Comment 39 Paolo Bonzini 2008-02-22 14:36:39 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate


>> I think you should use set_user_assembler_name instead of
>> SET_DECL_ASSEMBLER_NAME.
> 
> Nope. Doing this gives, for you C testcase with test and test_:
> 
> Undefined symbols:
>   "sinl$LDBL128", referenced from:
>       _test_ in ccYtxtFH.o

And for Fortran?

Actually, I think we're almost there.  You have to prepend an 
underscore.  Symbols in my 
/System/Library/Frameworks/System.framework/Versions/Current/System look 
like this:

90175020 T _cosl$LDBL128
901751c0 T _sinl$LDBL128
90175350 T _tanl$LDBL128
90176140 T _cbrtl$LDBL128
901764d0 T _sqrtl$LDBL128
90176c00 T _nanl$LDBL128
90176e70 T _nanl$LDBL64

You have to use set_user_assembler_name because: 1) that's what the 
__asm__ ("_sinl$LDBL128") directives do; 2) the assembler name is right, 
but the DECL_RTL is wrong in your dumps, and set_user_assembler_name 
resets the DECL_RTL so that it is recomputed.

Paolo
Comment 40 Uroš Bizjak 2008-02-22 14:49:06 UTC
This simple proof-of-concept patch works as expected:

--cut here--
Index: langhooks.c
===================================================================
--- langhooks.c (revision 132541)
+++ langhooks.c (working copy)
@@ -557,6 +557,9 @@ add_builtin_function (const char *name,
   gcc_assert (DECL_FUNCTION_CODE (decl) >= function_code);
   DECL_FUNCTION_CODE (decl)  = function_code;
 
+  if (function_code == BUILT_IN_SINL)
+    library_name = "sinl$LDBL128";
+
   if (library_name)
     {
       tree libname = get_identifier (library_name);
--cut here--

test_:
        jmp     sinl$LDBL128
test:
        jmp     sinl$LDBL128
Comment 41 Paolo Bonzini 2008-02-22 14:53:44 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate

ubizjak at gmail dot com wrote:
> ------- Comment #40 from ubizjak at gmail dot com  2008-02-22 14:49 -------
> This simple proof-of-concept patch works as expected:

I would very much prefer to see set_user_assembler_name first...

This excerpt from my math.h confirms that you need an underscore:

#if ( __WANT_LONG_DOUBLE_FORMAT__ - 0L == 128L )
#define __LIBMLDBL_COMPAT(sym) __asm("_" __STRING(sym) "$LDBL128")
#elif ( __WANT_LONG_DOUBLE_FORMAT__ - 0L == 64L )
#define __LIBMLDBL_COMPAT(sym) /* NOTHING */
#else
#define __LIBMLDBL_COMPAT(sym) /* NOTHING */
#endif

Paolo
Comment 42 Francois-Xavier Coudert 2008-02-22 14:59:45 UTC
(In reply to comment #39)
> Actually, I think we're almost there.  You have to prepend an 
> underscore.

You're right, and it works. I'm a bit afraid of doing so (if it's handled everywhere else, why isn't it handled for us?), but it does work.

(There is one exception: calls to cpowl generated by the Fortran front-end seem not to work, but it is a front-end issue: we don't treat cpow?() functions as builtins, probably a left-over from an earlier cleanup of that part of the front-end.)
Comment 43 Paolo Bonzini 2008-02-22 15:08:29 UTC
I don't think you have to be afraid.  Note that with your patch there wouldn't even be need to patch math.h -- in other words, your patch DTRT and it should have been done this way ever since the feature was introduced in the compiler.  Pity.
Comment 44 Francois-Xavier Coudert 2008-02-22 15:09:27 UTC
(In reply to comment #42)
> (There is one exception: calls to cpowl generated by the Fortran front-end seem
> not to work, but it is a front-end issue: we don't treat cpow?() functions as
> builtins, probably a left-over from an earlier cleanup of that part of the
> front-end.)

With the trivial fix to the front-end, it now works fine. Well, the testcase still doesn't pass, because Apple has a crazy erfl() implementation:

$ cat erf.c
#include <math.h>
#include <stdio.h>

int main(void)
{
  long double x = 1.45123231;
  double y = x;

  printf ("%g %g\n", (float) erfl(x), (float) erf(y));
  return 0;
}
$ gcc erf.c && ./a.out
-2.12115 0.959865

But at least, that's not my problem!
Comment 45 Jack Howarth 2008-02-22 15:11:58 UTC
I believe the code that assigns the function names in fortran is....

#define DO_DEFINE_MATH_BUILTIN(code, name, argtype, tbase) \
    gfc_define_builtin ("__builtin_" name "l", tbase##longdouble[argtype], \
                       BUILT_IN_ ## code ## L, name "l", true); \
    gfc_define_builtin ("__builtin_" name, tbase##double[argtype], \
                        BUILT_IN_ ## code, name, true); \
    gfc_define_builtin ("__builtin_" name "f", tbase##float[argtype], \
                        BUILT_IN_ ## code ## F, name "f", true);

#define DEFINE_MATH_BUILTIN(code, name, argtype) \
    DO_DEFINE_MATH_BUILTIN (code, name, argtype, mfunc_)

#define DEFINE_MATH_BUILTIN_C(code, name, argtype) \
    DO_DEFINE_MATH_BUILTIN (code, name, argtype, mfunc_) \
    DO_DEFINE_MATH_BUILTIN (C##code, "c" name, argtype, mfunc_c)

in gcc/fortran/f95-lang.c. The last time looked at Andrews proposed
patch, it setting all of the assembler names in a call to
darwin_patch_builtin but none of these are used by the final
assignment of DO_DEFINE_MATH_BUILTIN() in 95-lang.c. Also it
is possible that Andrew's original patch is regressed relative
to gcc trunk...

http://gcc.gnu.org/ml/fortran/2007-05/msg00499.html
http://gcc.gnu.org/ml/fortran/2007-05/msg00501.html
http://gcc.gnu.org/ml/fortran/2007-05/msg00505.html
http://gcc.gnu.org/ml/fortran/2007-05/msg00511.html
Comment 46 Paolo Bonzini 2008-02-22 15:20:04 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate

[off bugzilla]

I think FX is going to submit a patch soon.

Paolo
Comment 47 Francois-Xavier Coudert 2008-02-22 23:12:52 UTC
(In reply to comment #18)
> the list of functions to fix by Mike
> (where non-builtins have been removed, and variadic functions are fixed
> conditionaly on macos_version_min, as per Geoff's advice...

Modifying patch and restarting regtesting, because I just discovered that with -m64, the variadic functions should not be fixed at all (whatever the macos_version_min). I note it here for completeness of our records :)
Comment 48 Francois-Xavier Coudert 2008-02-23 18:42:49 UTC
Subject: Bug 25477

Author: fxcoudert
Date: Sat Feb 23 18:42:04 2008
New Revision: 132576

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132576
Log:
	PR target/25477

	* config/darwin-protos.h: Add darwin_patch_builtins prototype.
	* config/darwin-ppc-ldouble-patch.def: New file.
	* config/rs6000/darwin.h (SUBTARGET_INIT_BUILTINS): New macro.
	* config/rs6000/rs6000.c (rs6000_init_builtins): Call
	SUBTARGET_INIT_BUILTINS if defined.
	* config/darwin.c (darwin_patch_builtin,
	darwin_patch_builtins): New functions.

	* trans-expr.c (gfc_conv_power_op): Use BUILT_IN_CPOW{F,,L}.
	* f95-lang.c (gfc_init_builtin_functions): Define BUILT_IN_CPOW{F,,L}.
	* trans.h (gfor_fndecl_math_cpow, gfor_fndecl_math_cpowf,
	gfor_fndecl_math_cpowl10, gfor_fndecl_math_cpowl16): Remove.
	* trans-decl.c: Likewise.

	* gfortran.dg/large_real_kind_2.F90: Split testing of ERF and
	ERFC into gfortran.dg/large_real_kind_3.F90.
	* gfortran.dg/large_real_kind_3.F90: New test.

Added:
    trunk/gcc/config/darwin-ppc-ldouble-patch.def
    trunk/gcc/testsuite/gfortran.dg/large_real_kind_3.F90
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/darwin-protos.h
    trunk/gcc/config/darwin.c
    trunk/gcc/config/rs6000/darwin.h
    trunk/gcc/config/rs6000/rs6000.c
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/f95-lang.c
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/fortran/trans.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/large_real_kind_2.F90

Comment 49 Francois-Xavier Coudert 2008-02-23 19:00:18 UTC
To fix this completely, a little bit more work is required to check what should be done for nanl(), which is not handled by current patch. (I don't have time to look into it.) For more information, see http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01035.html
Comment 50 Uroš Bizjak 2008-02-25 11:29:42 UTC
Created attachment 15222 [details]
Addendum to the ldouble patch

Can someone regression test this patch on PPC darwin?

There are following changes:

- uses ACONCAT instead of alloca/strcpy/strcat
- removes wrong builtins (BUILT_IN_NEXTTOWARD and BUILT_IN_NEXTTOWARDF) from the definition
- adds BUILT_IN_NAN (after [1] goes into mainline)
- some whitspace fixes

[1] http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01139.html
Comment 51 Dominique d'Humieres 2008-02-25 12:45:38 UTC
In the patch from comment #50

s/+#esle/+#else/
Comment 52 Dominique d'Humieres 2008-02-25 12:57:28 UTC
The test case gfortran.dg/large_real_kind_3.F90 fails on powerpc-apple-darwin9 because it is xfailed for freebsd and not for darwin:

! { dg-xfail-if "" { "*-*-freebsd*" } { "*" }  { "" } }

Comment 53 Francois-Xavier Coudert 2008-02-25 13:03:51 UTC
(In reply to comment #52)
> The test case gfortran.dg/large_real_kind_3.F90 fails on powerpc-apple-darwin9
> because it is xfailed for freebsd and not for darwin

Oops, sorry, fixed by rev. 132621.
Comment 54 Uroš Bizjak 2008-02-25 15:10:05 UTC
(In reply to comment #51)
> In the patch from comment #50
> 
> s/+#esle/+#else/

Ops.

So, with this fixlet, does the patch work OK on ppc-darwin, so I can post it to gcc-patches@ (Yes, I'll write a ChangeLog ;) ?
Comment 55 Dominique d'Humieres 2008-02-25 15:25:55 UTC
In reply to comment #54:
>does the patch work OK on ppc-darwin?

I don't have the time to do the test right now. I'll try tonight.

Comment 56 Jack Howarth 2008-02-25 17:09:36 UTC
My bad...

  newname = ACONCAT ("_", IDENTIFIER_POINTER (sym), "$LDBL128", NULL);

needs to be...

  newname = ACONCAT (("_", IDENTIFIER_POINTER (sym), "$LDBL128", NULL));

...otherwise you get the following compiler errors...

gcc -c   -g -fkeep-inline-functions -DIN_GCC   -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -fno-common   -DHAVE_CONFIG_H -I. -I. -I../../gcc-4.3.0-RC-20080222/gcc -I../../gcc-4.3.0-RC-20080222/gcc/. -I../../gcc-4.3.0-RC-20080222/gcc/../include -I../../gcc-4.3.0-RC-20080222/gcc/../libcpp/include -I/sw/include  -I../../gcc-4.3.0-RC-20080222/gcc/../libdecnumber -I../../gcc-4.3.0-RC-20080222/gcc/../libdecnumber/dpd -I../libdecnumber -I/sw/include   -I. -I. -I../../gcc-4.3.0-RC-20080222/gcc -I../../gcc-4.3.0-RC-20080222/gcc/. -I../../gcc-4.3.0-RC-20080222/gcc/../include -I../../gcc-4.3.0-RC-20080222/gcc/../libcpp/include -I/sw/include  -I../../gcc-4.3.0-RC-20080222/gcc/../libdecnumber -I../../gcc-4.3.0-RC-20080222/gcc/../libdecnumber/dpd -I../libdecnumber ../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c
../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c:1751:69: error: macro "ACONCAT" passed 4 arguments, but takes just 1
../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c: In function 'darwin_patch_builtin':
../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c:1751: error: 'ACONCAT' undeclared (first use in this function)
../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c:1751: error: (Each undeclared identifier is reported only once
../../gcc-4.3.0-RC-20080222/gcc/config/darwin.c:1751: error: for each function it appears in.)
Comment 57 Jack Howarth 2008-02-26 17:15:26 UTC
Uros,
   You changes in d.diff.txt are fine with the exception of the missing extra set of parentheses for the ACONCAT macro call. I have tested the patch with those corrections and powerpc-apple-darwin9 properly uses $LDBL128 with it. Can you
post the corrected patch to gcc-patches? I would like to post a backport of 
FX and your changes for gcc 4.3.
                 Jack
Comment 58 uros 2008-02-27 17:30:45 UTC
Subject: Bug 25477

Author: uros
Date: Wed Feb 27 17:29:58 2008
New Revision: 132723

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132723
Log:
        PR target/25477
        * config/darwin-ppc-ldouble-patch.def (BUILT_IN_NANL): Add.
        (BUILT_IN_NEXTTOWARD): Remove.
        (BUILT_IN_NEXTTOWARDF): Ditto.
        * config/darwin.c (darwin_patch_builtin): Use ACONCAT instead of
        alloca/strcpy/strcat.  Remove commented-out code.  Fix whitespace.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/darwin-ppc-ldouble-patch.def
    trunk/gcc/config/darwin.c

Comment 59 Uroš Bizjak 2008-02-27 17:32:28 UTC
Fixed.
Comment 60 Dominique d'Humieres 2008-02-27 21:26:31 UTC
(In reply to comment #53)

> Oops, sorry, fixed by rev. 132621.

Now I get:

XPASS: gfortran.dg/large_real_kind_3.F90  -O0  (test for excess errors)
FAIL: gfortran.dg/large_real_kind_3.F90  -O0  execution test
XPASS: gfortran.dg/large_real_kind_3.F90  -O1  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O2  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer -funroll-loops  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -g  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -Os  (test for excess errors)

in both 32 and 64 bit modes. I think the xpass are normal as the optimized dump is:

;; Function MAIN__ (MAIN__)

Analyzing Edge Insertions.
MAIN__ ()
{
  static integer(kind=4) options.0[7] = {68, 127, 0, 0, 0, 1, 0};

<bb 2>:
  _gfortran_set_options (7, &options.0);
  if (2.9236712223862218652692294906906497118927045376e-17 / 9.59864575062830072127439962059725075960159301758e-1 > 9.99999993922529029077850282192230224609375e-9)
    goto <bb 3>;
  else
    goto <bb 4>;

<bb 3>:
  _gfortran_abort ();

<bb 4>:
  if (5.87424647807422190473069128537003046257953999681e-17 / 1.13897071699133300981543470697943121194839477539e+0 > 9.99999993922529029077850282192230224609375e-9)
    goto <bb 5>;
  else
    goto <bb 6>;

<bb 5>:
  _gfortran_abort ();

<bb 6>:
  return;

}

Also I don't understand why there is a test for "excess errors".

Comment 61 Paolo Bonzini 2008-02-27 21:32:55 UTC
Subject: Re:  builtin functions should use $LDBL128 suffix
 on darwin when appropriate

I think it depends on the version of mpfr that you have installed?

> Also I don't understand why there is a test for "excess errors".

Errors/warnings not matching a dg-error.
Comment 62 Francois-Xavier Coudert 2008-02-27 22:28:30 UTC
That new testcase fails under Darwin at -O0 (because of a faulty libm) and passes with optimization turned on (because the middle-end is very clever). So, there two solutions are: either have 1 FAIL (-O0) and the rest PASS, like I had before; or have 1XFAIL and the rest XPASS, which is was we have after my last commit.

The first situation was clearly better, but I was at work when Dominique emailed my after my first commit, saying the new testcase FAILed, and I got confused. I think I'll revert to the old situation, and we'll live with that one FAIL until Apple fixes the bug or the universe stops expanding and condenses into a single point of pure energy, whichever happens first. (My money is on the second guess.)
Comment 63 Dominique d'Humieres 2008-02-27 22:52:29 UTC
(In reply to comment #62)
I have nothing against FAIL (I am not fond of XFAIL that in my opinion hide the problem). I reported the failures because I understood that they were not expected. If now they will, it fine with me.

Comment 64 uros 2008-02-28 07:09:44 UTC
Subject: Bug 25477

Author: uros
Date: Thu Feb 28 07:08:51 2008
New Revision: 132737

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132737
Log:
	PR target/25477
	* gcc/config/darwin-protos.h: Add darwin_patch_builtins prototype.
	* gcc/config/darwin-ppc-ldouble-patch.def: New file.
	* gcc/config/rs6000/darwin.h (SUBTARGET_INIT_BUILTINS): New macro.
	* gcc/config/rs6000/rs6000.c (rs6000_init_builtins): Call
	SUBTARGET_INIT_BUILTINS if defined.
	* gcc/config/darwin.c (darwin_patch_builtin,
	darwin_patch_builtins): New functions. 

fortran/ChangeLog:

	PR target/25477
	* trans-expr.c (gfc_conv_power_op): Use BUILT_IN_CPOW{F,,L}.
	* f95-lang.c (gfc_init_builtin_functions): Define BUILT_IN_CPOW{F,,L}.
	* trans.h (gfor_fndecl_math_cpow, gfor_fndecl_math_cpowf,
	gfor_fndecl_math_cpowl10, gfor_fndecl_math_cpowl16): Remove.
	* trans-decl.c: Likewise.

testsuite/ChangeLog:

	PR libfortran/24685
	* gfortran.dg/large_real_kind_form_io_2.f90: XFAIL on
	powerpc*-apple-darwin*.
	* gfortran.dg/large_real_kind_2.F90: Split testing of ERF and
	ERFC into gfortran.dg/large_real_kind_3.F90.
	* gfortran.dg/large_real_kind_3.F90: New test.


Added:
    branches/gcc-4_3-branch/gcc/config/darwin-ppc-ldouble-patch.def
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/large_real_kind_3.F90
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/config/darwin-protos.h
    branches/gcc-4_3-branch/gcc/config/darwin.c
    branches/gcc-4_3-branch/gcc/config/rs6000/darwin.h
    branches/gcc-4_3-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_3-branch/gcc/fortran/ChangeLog
    branches/gcc-4_3-branch/gcc/fortran/f95-lang.c
    branches/gcc-4_3-branch/gcc/fortran/trans-decl.c
    branches/gcc-4_3-branch/gcc/fortran/trans-expr.c
    branches/gcc-4_3-branch/gcc/fortran/trans.h
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/large_real_kind_2.F90
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/large_real_kind_form_io_2.f90

Comment 65 Uroš Bizjak 2008-02-28 11:29:23 UTC
(In reply to comment #62)

> The first situation was clearly better, but I was at work when Dominique
> emailed my after my first commit, saying the new testcase FAILed, and I got
> confused. I think I'll revert to the old situation, and we'll live with that
> one FAIL until Apple fixes the bug or the universe stops expanding and
> condenses into a single point of pure energy, whichever happens first. (My
> money is on the second guess.)

Please note, that the syntax of dg-xfail-if is

dg-xfail-if message {targets} {include} {exclude}

so perhaps adding another dg-xfail-if line with someting like

dg-xfail-if "apple libc problems" { powerpc*-apple-darwin* } {"*" } {"-O0"}

would produce clean run.

Comment 66 Jack Howarth 2008-02-28 14:39:01 UTC
I find that...

! { dg-xfail-if "" { "*-*-freebsd*" } { "*" }  { "" } }
! { dg-xfail-if "apple libc problems" { powerpc*-apple-darwin* } {"*" } {"-O0"} }

in gfortran.dg/large_real_kind_3.F90, still produces...

FAIL: gfortran.dg/large_real_kind_3.F90  -O0  execution test
XPASS: gfortran.dg/large_real_kind_3.F90  -O1  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O2  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer -funroll-loops  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -O3 -g  (test for excess errors)
XPASS: gfortran.dg/large_real_kind_3.F90  -Os  (test for excess errors)

So it seems that the "-O0" part of the dg-xfail-if isn't being honored.
Comment 67 Uroš Bizjak 2008-02-28 14:54:34 UTC
(In reply to comment #66)

> in gfortran.dg/large_real_kind_3.F90, still produces...

It should be in reverse:

! { dg-xfail-if "libc bug" { "powerpc*-apple-darwin*" } { "-O0" }  { "" } }

 -> http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01412.html
Comment 68 uros 2008-02-29 20:56:05 UTC
Subject: Bug 25477

Author: uros
Date: Fri Feb 29 20:55:19 2008
New Revision: 132777

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132777
Log:
        PR middle-end/19984
        * builtins.def (BUILT_IN_NAN): Define as c99 builtin
        using DEF_C99_BUILTIN.
        (BUILT_IN_NANF): Ditto.
        (BUILT_IN_NANL): Ditto.

testsuite/ChangeLog:

        PR middle-end/19984
        * gcc.dg/pr19984.c: New test.
        * gcc.dg/dfp/compare-special.h: Use _nan instead of nan as the
        name of the variable.

        PR target/25477
        * config/darwin-ppc-ldouble-patch.def (BUILT_IN_NANL): Add.


Added:
    branches/gcc-4_3-branch/gcc/testsuite/gcc.dg/pr19984.c
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/builtins.def
    branches/gcc-4_3-branch/gcc/config/darwin-ppc-ldouble-patch.def
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_3-branch/gcc/testsuite/gcc.dg/dfp/compare-special.h