On Tue, Oct 15, 2013 at 9:59 AM, Richard Sandiford
<rdsandif...@googlemail.com> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>>>> Honza? For tailr this boils down to symtab_semantically_equivalent_p ().
>>>> I suppose we don't want to change that but eventually special case
>>>> builtins in tailr, thus
>>>>
>>>> Index: gcc/tree-tailcall.c
>>>> ===================================================================
>>>> --- gcc/tree-tailcall.c (revision 203409)
>>>> +++ gcc/tree-tailcall.c (working copy)
>>>> @@ -446,7 +446,9 @@ find_tail_calls (basic_block bb, struct
>>>> /* We found the call, check whether it is suitable. */
>>>> tail_recursion = false;
>>>> func = gimple_call_fndecl (call);
>>>> - if (func && recursive_call_p (current_function_decl, func))
>>>> + if (func
>>>> + && ! DECL_BUILT_IN (func)
>>>> + && recursive_call_p (current_function_decl, func))
>>>> {
>>>> tree arg;
>>>>
>>>> which makes -O2 not turn it into an infinite loop (possibly also applies
>>>> to the original code with the alias declaration?)
>>>
>>> If that's OK then I'm certainly happy with it :-)
>>
>> I'm happy with the tailcall change (if you can do the testing ... ;))
>
> Thanks.
>
>>> FWIW, the alias case
>>> was first optimised from __sync_synchronize->sync_synchronize, before it
>>> got converted into a tail call. That happened very early in the pipeline
>>> and I suppose would stop the built-in expansion from kicking in,
>>> even with tail calls disabled. But if we say that:
>>>
>>> foo () { foo (); }
>>>
>>> is a supported way of defining out-of-line versions of built-in foo,
>>> then that's much more convenient than the aliases anyway.
>>
>> Btw, if it is supported then we should add a testcase that makes sure
>> we don't regress for this. (I wouldn't say we should document this
>> "feature" just in case we want to decide it's not the way we want it
>> in the future).
>
> OK, I adjusted the x86 test I posted on gcc@ (and fixed the \; problem
> Jakub pointed out).
>
> Tested on x86_64-linux-gnu and mips64-linux-gnu. OK to install?
Ok for the tailcall parts and the testcase - I'd prefer someone else to
ack the libgcc change. CCing maintainer.
Thanks,
Richard.
> Thanks,
> Richard
>
>
> gcc/
> 2013-10-15 Richard Biener <rguent...@suse.de>
>
> * tree-tailcall.c (find_tail_calls): Don't use tail-call recursion
> for built-in functions.
>
> gcc/testsuite/
> * gcc.dg/torture/builtin-self.c: New file.
>
> libgcc/
> * sync.c: Remove static aliases and define each function directly
> under its real name.
>
> Index: gcc/tree-tailcall.c
> ===================================================================
> --- gcc/tree-tailcall.c 2013-10-15 08:52:07.358853220 +0100
> +++ gcc/tree-tailcall.c 2013-10-15 08:53:06.980419473 +0100
> @@ -446,7 +446,9 @@ find_tail_calls (basic_block bb, struct
> /* We found the call, check whether it is suitable. */
> tail_recursion = false;
> func = gimple_call_fndecl (call);
> - if (func && recursive_call_p (current_function_decl, func))
> + if (func
> + && !DECL_BUILT_IN (func)
> + && recursive_call_p (current_function_decl, func))
> {
> tree arg;
>
> Index: gcc/testsuite/gcc.dg/torture/builtin-self.c
> ===================================================================
> --- /dev/null 2013-10-13 08:29:45.608935301 +0100
> +++ gcc/testsuite/gcc.dg/torture/builtin-self.c 2013-10-15 08:58:00.064045777
> +0100
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* Check that we can use this idiom to define out-of-line copies of built-in
> + functions. This is used by libgcc/sync.c, for example. */
> +void __sync_synchronize (void)
> +{
> + __sync_synchronize ();
> +}
> +/* { dg-final { scan-assembler "__sync_synchronize" } } */
> +/* { dg-final { scan-assembler "\t(lock|mfence)" } } */
> +/* { dg-final { scan-assembler-not "\tcall" } } */
> Index: libgcc/sync.c
> ===================================================================
> --- libgcc/sync.c 2013-10-15 08:52:07.358853220 +0100
> +++ libgcc/sync.c 2013-10-15 08:53:06.981419482 +0100
> @@ -72,22 +72,22 @@ Software Foundation; either version 3, o
> TYPE is a type that has UNITS bytes. */
>
> #define DEFINE_V_PV(NAME, UNITS, TYPE) \
> - static TYPE \
> - NAME##_##UNITS (TYPE *ptr, TYPE value) \
> + TYPE \
> + __##NAME##_##UNITS (TYPE *ptr, TYPE value) \
> { \
> return __##NAME (ptr, value); \
> }
>
> -#define DEFINE_V_PVV(NAME, UNITS, TYPE) \
> - static TYPE \
> - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \
> +#define DEFINE_V_PVV(NAME, UNITS, TYPE)
> \
> + TYPE \
> + __##NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \
> { \
> return __##NAME (ptr, value1, value2); \
> }
>
> #define DEFINE_BOOL_PVV(NAME, UNITS, TYPE) \
> - static _Bool \
> - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \
> + _Bool
> \
> + __##NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \
> { \
> return __##NAME (ptr, value1, value2); \
> }
> @@ -118,9 +118,7 @@ #define local_sync_lock_test_and_set DEF
> #define DEFINE1(NAME, UNITS, TYPE) \
> static int unused[sizeof (TYPE) == UNITS ? 1 : -1] \
> __attribute__((unused)); \
> - local_##NAME (NAME, UNITS, TYPE); \
> - typeof (NAME##_##UNITS) __##NAME##_##UNITS \
> - __attribute__((alias (#NAME "_" #UNITS)));
> + local_##NAME (NAME, UNITS, TYPE);
>
> /* As above, but performing macro expansion on the arguments. */
> #define DEFINE(NAME, UNITS, TYPE) DEFINE1 (NAME, UNITS, TYPE)
> @@ -167,13 +165,11 @@ DEFINE (FN, 8, UOItype)
>
> #if defined Lsync_synchronize
>
> -static void
> -sync_synchronize (void)
> +void
> +__sync_synchronize (void)
> {
> __sync_synchronize ();
> }
> -typeof (sync_synchronize) __sync_synchronize \
> - __attribute__((alias ("sync_synchronize")));
>
> #endif
>
