Bug 25477

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

(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.

No longer working on this, too much troubles are causing to me to fix Darwin bugs.

Mine, this is will be one of my last Darwin specific patches.  Hopefully PR 23504 gets resolved with it too.

I am no longer working on this one.

*** Bug 25850 has been marked as a duplicate of this bug. ***

*** Bug 23504 has been marked as a duplicate of this bug. ***

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.

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.

(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.

Geoff,
     Okay. Any chance you or Mike could take a stab at patching this bug in time for gcc 4.2?
               Jack

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?

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

How does the header manage to fix this?

(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

So this line of add_builtin_function

      tree libname = get_identifier (library_name);

should instead invoke a target hook (defaulting to get_identifier)?

(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.

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?

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

(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.

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.

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.

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.

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

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.

> 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.

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

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?

>> 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

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

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

> 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).

(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".

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

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)

I think you should use set_user_assembler_name instead of SET_DECL_ASSEMBLER_NAME.

(Not that I knew that this morning, of course).

(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

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

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

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

(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.)

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.

(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!

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

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

(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 :)

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

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

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

In the patch from comment #50

s/+#esle/+#else/

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*" } { "*" }  { "" } }

(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.

(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 ;) ?

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.

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.)

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

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

Fixed.

(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".

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.

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.)

(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.

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

(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.

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.

(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

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