RFA: Remove alias usage from libgcc/sync.c

Richard Biener <richard.guenther@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?

Thanks,
Richard


gcc/
2013-10-15  Richard Biener  <rguenther@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.

On Tue, Oct 15, 2013 at 9:59 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Biener <richard.guenther@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  <rguenther@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
>

On Tue, Oct 15, 2013 at 2:18 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
>
> Ok for the tailcall parts and the testcase - I'd prefer someone else to
> ack the libgcc change.  CCing maintainer.

The libgcc patch is missing the updates to the comments.  This code is
confusing enough as it is, having incorrect comments wouldn't help.

Ian