[bionic] Add -foptimize-sincos

Bionic prior to Gingerbread doesn't support sincos*, but upstream GCC
enables sincos optimization for OPTION_BIONIC unconditionally since
4.6.  I'd like to propose a new flag -foptimize-sincos for NDK to
maintain backward compatibility.

1. For BIONIC: sincos optimization is disabled by default.  Apps built
for Gingerbread+ or AOSP platform build which uses the same compiler
as NDK, can add -foptimize-sincos to enhance performance.  (although
it's likely that only benchmarks may benefit)

2. Other C libs aren't affected.  sincos optimization is enabled for
GLIBC and disabled for othres, regardless of this flag.

 #undef TARGET_HAS_BIONIC

On Fri, May 24, 2013 at 12:53 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
> Bionic prior to Gingerbread doesn't support sincos*, but upstream GCC
> enables sincos optimization for OPTION_BIONIC unconditionally since
> 4.6.  I'd like to propose a new flag -foptimize-sincos for NDK to
> maintain backward compatibility.
>
> 1. For BIONIC: sincos optimization is disabled by default.  Apps built
> for Gingerbread+ or AOSP platform build which uses the same compiler
> as NDK, can add -foptimize-sincos to enhance performance.  (although
> it's likely that only benchmarks may benefit)
>
> 2. Other C libs aren't affected.  sincos optimization is enabled for
> GLIBC and disabled for othres, regardless of this flag.

That's a pretty awful option name for one that makes us assume the target
C library has a sincos function.

I'd rather think about a way to specify, for all known builtins, whether GCC
should generate calls to such function where they are not in the source
program.  That is, similar to how we have -f[no-]builtin-FOO introduce
-f[no-]libc-FOO (with a better name for 'libc'?).  That way there would be
a way to specify that -floop-distribute-patterns should not produce calls
to memset () for example.

Richard.

> ====
> Index: gcc/common.opt
> ===================================================================
> --- gcc/common.opt (revision 199277)
> +++ gcc/common.opt (working copy)
> @@ -1591,6 +1591,10 @@
>  Common Report Var(flag_optimize_sibling_calls) Optimization
>  Optimize sibling and tail recursive calls
>
> +foptimize-sincos
> +Common Report Var(flag_optimize_sincos) Optimization
> +Optimize calls to sin() and cos() with the same argument to sincos()
> +
>  fpartial-inlining
>  Common Report Var(flag_partial_inlining)
>  Perform partial inlining
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi (revision 199277)
> +++ gcc/doc/invoke.texi (working copy)
> @@ -388,7 +388,7 @@
>  -fno-sched-interblock -fno-sched-spec -fno-signed-zeros @gol
>  -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss @gol
>  -fomit-frame-pointer -foptimize-register-move -foptimize-sibling-calls @gol
> --fpartial-inlining -fpeel-loops -fpredictive-commoning @gol
> +-foptimize-sincos -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol
>  -fprefetch-loop-arrays -fprofile-report @gol
>  -fprofile-correction -fprofile-dir=@var{path} -fprofile-generate @gol
>  -fprofile-generate=@var{path} @gol
> @@ -6586,6 +6586,7 @@
>  -fipa-profile @gol
>  -fipa-reference @gol
>  -fmerge-constants
> +-foptimize-sincos @gol
>  -fsplit-wide-types @gol
>  -ftree-bit-ccp @gol
>  -ftree-builtin-call-dce @gol
> @@ -6772,6 +6773,12 @@
>
>  Enabled at levels @option{-O2}, @option{-O3}, @option{-Os}.
>
> +@item -foptimize-sincos
> +@opindex foptimize-sincos
> +Optimize calls to sin() and cos() with the same argument to sincos()
> +
> +Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
> +
>  @item -fno-inline
>  @opindex fno-inline
>  Do not expand any functions inline apart from those marked with
> Index: gcc/config/linux.h
> ===================================================================
> --- gcc/config/linux.h (revision 199277)
> +++ gcc/config/linux.h (working copy)
> @@ -102,7 +102,7 @@
>
>  /* Whether we have sincos that follows the GNU extension.  */
>  #undef TARGET_HAS_SINCOS
> -#define TARGET_HAS_SINCOS (OPTION_GLIBC || OPTION_BIONIC)
> +#define TARGET_HAS_SINCOS (OPTION_GLIBC || (OPTION_BIONIC &&
> flag_optimize_sincos))
>
>  /* Whether we have Bionic libc runtime */
>  #undef TARGET_HAS_BIONIC

On Fri, May 24, 2013 at 02:10:18PM +0200, Richard Biener wrote:
> That's a pretty awful option name for one that makes us assume the target
> C library has a sincos function.
> 
> I'd rather think about a way to specify, for all known builtins, whether GCC
> should generate calls to such function where they are not in the source
> program.  That is, similar to how we have -f[no-]builtin-FOO introduce
> -f[no-]libc-FOO (with a better name for 'libc'?).  That way there would be
> a way to specify that -floop-distribute-patterns should not produce calls
> to memset () for example.

Yeah.  Or we could be more aggressive at producing stpcpy, mempcpy etc.
calls where it could be beneficial and have a way to disable that.
What we currently do for stpcpy is c/c-decl.c (merge_decl) and
cp/decl.c (duplicate_decls) has code that if not -fno-builtin-stpcpy and
a compatible prototype is seen for stpcpy, then we allow it to be generated
implicitly (set_builtin_decl_implicit_p (fncode, true);).
Yeah, we could do the same for sincos, I bet most of the people don't
prototype sin or cos themselves but include <math.h>, but in all cases
it means the compiler will do it only if stpcpy resp. sincos is actually
prototyped in the headers (right feature test macros for that).

	Jakub

Richard, the target hook (libc_has_function) for what you described is
waiting for a review:
http://gcc.gnu.org/ml/gcc-patches/2013-03/msg01201.html

However, it doesn't have command line options support.

Alexander


2013/5/24 Richard Biener <richard.guenther@gmail.com>:
> On Fri, May 24, 2013 at 12:53 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
>> Bionic prior to Gingerbread doesn't support sincos*, but upstream GCC
>> enables sincos optimization for OPTION_BIONIC unconditionally since
>> 4.6.  I'd like to propose a new flag -foptimize-sincos for NDK to
>> maintain backward compatibility.
>>
>> 1. For BIONIC: sincos optimization is disabled by default.  Apps built
>> for Gingerbread+ or AOSP platform build which uses the same compiler
>> as NDK, can add -foptimize-sincos to enhance performance.  (although
>> it's likely that only benchmarks may benefit)
>>
>> 2. Other C libs aren't affected.  sincos optimization is enabled for
>> GLIBC and disabled for othres, regardless of this flag.
>
> That's a pretty awful option name for one that makes us assume the target
> C library has a sincos function.
>
> I'd rather think about a way to specify, for all known builtins, whether GCC
> should generate calls to such function where they are not in the source
> program.  That is, similar to how we have -f[no-]builtin-FOO introduce
> -f[no-]libc-FOO (with a better name for 'libc'?).  That way there would be
> a way to specify that -floop-distribute-patterns should not produce calls
> to memset () for example.
>
> Richard.
>
>> ====
>> Index: gcc/common.opt
>> ===================================================================
>> --- gcc/common.opt (revision 199277)
>> +++ gcc/common.opt (working copy)
>> @@ -1591,6 +1591,10 @@
>>  Common Report Var(flag_optimize_sibling_calls) Optimization
>>  Optimize sibling and tail recursive calls
>>
>> +foptimize-sincos
>> +Common Report Var(flag_optimize_sincos) Optimization
>> +Optimize calls to sin() and cos() with the same argument to sincos()
>> +
>>  fpartial-inlining
>>  Common Report Var(flag_partial_inlining)
>>  Perform partial inlining
>> Index: gcc/doc/invoke.texi
>> ===================================================================
>> --- gcc/doc/invoke.texi (revision 199277)
>> +++ gcc/doc/invoke.texi (working copy)
>> @@ -388,7 +388,7 @@
>>  -fno-sched-interblock -fno-sched-spec -fno-signed-zeros @gol
>>  -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss @gol
>>  -fomit-frame-pointer -foptimize-register-move -foptimize-sibling-calls @gol
>> --fpartial-inlining -fpeel-loops -fpredictive-commoning @gol
>> +-foptimize-sincos -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol
>>  -fprefetch-loop-arrays -fprofile-report @gol
>>  -fprofile-correction -fprofile-dir=@var{path} -fprofile-generate @gol
>>  -fprofile-generate=@var{path} @gol
>> @@ -6586,6 +6586,7 @@
>>  -fipa-profile @gol
>>  -fipa-reference @gol
>>  -fmerge-constants
>> +-foptimize-sincos @gol
>>  -fsplit-wide-types @gol
>>  -ftree-bit-ccp @gol
>>  -ftree-builtin-call-dce @gol
>> @@ -6772,6 +6773,12 @@
>>
>>  Enabled at levels @option{-O2}, @option{-O3}, @option{-Os}.
>>
>> +@item -foptimize-sincos
>> +@opindex foptimize-sincos
>> +Optimize calls to sin() and cos() with the same argument to sincos()
>> +
>> +Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
>> +
>>  @item -fno-inline
>>  @opindex fno-inline
>>  Do not expand any functions inline apart from those marked with
>> Index: gcc/config/linux.h
>> ===================================================================
>> --- gcc/config/linux.h (revision 199277)
>> +++ gcc/config/linux.h (working copy)
>> @@ -102,7 +102,7 @@
>>
>>  /* Whether we have sincos that follows the GNU extension.  */
>>  #undef TARGET_HAS_SINCOS
>> -#define TARGET_HAS_SINCOS (OPTION_GLIBC || OPTION_BIONIC)
>> +#define TARGET_HAS_SINCOS (OPTION_GLIBC || (OPTION_BIONIC &&
>> flag_optimize_sincos))
>>
>>  /* Whether we have Bionic libc runtime */
>>  #undef TARGET_HAS_BIONIC

On Fri, May 24, 2013 at 2:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, May 24, 2013 at 02:10:18PM +0200, Richard Biener wrote:
>> That's a pretty awful option name for one that makes us assume the target
>> C library has a sincos function.
>>
>> I'd rather think about a way to specify, for all known builtins, whether GCC
>> should generate calls to such function where they are not in the source
>> program.  That is, similar to how we have -f[no-]builtin-FOO introduce
>> -f[no-]libc-FOO (with a better name for 'libc'?).  That way there would be
>> a way to specify that -floop-distribute-patterns should not produce calls
>> to memset () for example.
>
> Yeah.  Or we could be more aggressive at producing stpcpy, mempcpy etc.
> calls where it could be beneficial and have a way to disable that.
> What we currently do for stpcpy is c/c-decl.c (merge_decl) and
> cp/decl.c (duplicate_decls) has code that if not -fno-builtin-stpcpy and
> a compatible prototype is seen for stpcpy, then we allow it to be generated
> implicitly (set_builtin_decl_implicit_p (fncode, true);).

But for example memset/memcpy always have that set, even if no prototype
is in the source.  So, is that decl_implicit_p really supposed to tell us
whether we've seen a compatible prototype?

> Yeah, we could do the same for sincos, I bet most of the people don't
> prototype sin or cos themselves but include <math.h>, but in all cases
> it means the compiler will do it only if stpcpy resp. sincos is actually
> prototyped in the headers (right feature test macros for that).

Right, that would be good enough for C and C++ - but what to do for Fortran?

Richard.

>         Jakub

On Fri, May 24, 2013 at 02:23:45PM +0200, Richard Biener wrote:
> But for example memset/memcpy always have that set, even if no prototype
> is in the source.  So, is that decl_implicit_p really supposed to tell us
> whether we've seen a compatible prototype?

decl_implicit_p isn't whether we've seen a compatible prototype, but whether
it is ok for the compiler to make calls to that function, even when there
were none in the source.  Usually this comes from how we define the builtin
in builtins.def, whether it is a standard required function or some
extension beyond the standard.  And for stpcpy right now we are using this
way (kind of hack).

> Right, that would be good enough for C and C++ - but what to do for Fortran?

Sure.  So what about have a -f{,no-}builtin-implicit-{builtin_name}
with the default as is right now, that would just tweak decl_implicit_p if
decl_explicit_p is true.

	Jakub

On Fri, May 24, 2013 at 2:28 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, May 24, 2013 at 02:23:45PM +0200, Richard Biener wrote:
>> But for example memset/memcpy always have that set, even if no prototype
>> is in the source.  So, is that decl_implicit_p really supposed to tell us
>> whether we've seen a compatible prototype?
>
> decl_implicit_p isn't whether we've seen a compatible prototype, but whether
> it is ok for the compiler to make calls to that function, even when there
> were none in the source.  Usually this comes from how we define the builtin
> in builtins.def, whether it is a standard required function or some
> extension beyond the standard.  And for stpcpy right now we are using this
> way (kind of hack).
>
>> Right, that would be good enough for C and C++ - but what to do for Fortran?
>
> Sure.  So what about have a -f{,no-}builtin-implicit-{builtin_name}
> with the default as is right now, that would just tweak decl_implicit_p if
> decl_explicit_p is true.

That works for me.  We could still have a way for targets to override the
default here.

Richard.

>         Jakub