Re: atomic update of profile counters (issue7000044)
> 2013-11-19 Rong Xu
>
> * gcc/gcov-io.h: Add atomic function macros for compiler use.
> * gcc/common.opt (fprofile-generate-atomic): New option.
> * gcc/tree-profile.c (gimple_init_edge_profiler): Support for
> atomic counter update.
> (gimple_gen_edge_profiler): Ditto.
> * libgcc/libgcov-profiler.c
> (__gcov_interval_profiler_atomic): Ditto.
> (__gcov_pow2_profiler_atomic): Ditto.
> (__gcov_one_value_profiler_body_atomic): Ditto.
> (__gcov_one_value_profiler_atomic): Ditto.
> (__gcov_indirect_call_profiler_atomic): Ditto.
> (__gcov_indirect_call_profiler_v2_atomic): Ditto.
> (__gcov_time_profiler_atomic): Ditto.
> (__gcov_average_profiler_atomic): Ditto.
> * gcc/gcc.c: Link libatomic when -fprofile-generate-atomic used.
> * libgcc/Makefile.in: Add atomic objects.
>
> Index: gcc/common.opt
> ===
> --- gcc/common.opt(revision 205053)
> +++ gcc/common.opt(working copy)
> @@ -1684,6 +1684,15 @@ fprofile-correction
> Common Report Var(flag_profile_correction)
> Enable correction of flow inconsistent profile data input
>
> +; fprofile-generate-atomic=0: default and disable atomical update.
> +; fprofile-generate-atomic=1: atomically update edge profile counters.
> +; fprofile-generate-atomic=2: atomically update value profile counters.
> +; fprofile-generate-atomic=3: atomically update edge and value profile
> counters.
> +; other values will be ignored (fall back to the default of 0).
> +fprofile-generate-atomic=
> +Common Joined UInteger Report Var(flag_profile_generate_atomic) Init(3)
> Optimization
> +fprofile-generate-atomic=[0..3] Atomical increments of profile counters.
Instead magic numbers I would preffer flags, like "edge" and "value"
and permiting combinations like -ffprofile-generate-atomic=edge,value
I wonder about the option name, I suppose this option also combine with
-ftest-coverage
(and no longer too useful -fprofile-arcs), so the profile-generate prefix may
be bit
misleading here (giving an impression that it is not useful in this case).
But I can't think of something really better.
Other thing I wonder about is that we may want to implement alternative
solution with
TLS or smaller per-thread buffers and locked updates. It would be bit
difficult to
extend -fprofile-generate-atomic to this...
> @@ -155,6 +155,9 @@ gimple_init_edge_profiler (void)
>tree ic_profiler_fn_type;
>tree average_profiler_fn_type;
>tree time_profiler_fn_type;
> + const char *fn_name;
> + bool profile_gen_value_atomic = (flag_profile_generate_atomic == 2 ||
> + flag_profile_generate_atomic == 3);
>
>if (!gcov_type_node)
> {
Will this work with optimization attributes?
The rest of patch looks OK, lets settle option name and get it in.
Sorry for dropping the ball for 4.8.
Re: atomic update of profile counters (issue7000044)
OK. Sorry for miss-reading the message.
In that case, linking in libatomic becomes a separate issue. We don't
need to touch gcc.c in this patch.
Thanks,
-Rong
On Wed, Nov 20, 2013 at 2:19 PM, Andrew Pinski wrote:
> On Wed, Nov 20, 2013 at 2:07 PM, Rong Xu wrote:
>> Joseph and Andrew, thanks for the suggestion. That's really helpful.
>>
>> Here is the new patch for gcc.c.
>> Basically, it's just what you have suggested: enclosing -latomic with
>> --as-needed, and using macros.
>> For the case of no --as-needed support, I use static link. (just found
>> that some code already using this in the SPEC).
>> I'm flexible on this part -- if you think this is unnecessary, I can remove.
>
>
> I think Joseph's suggestion was also to include -latomic even when not
> generating atomic profiling due to the C11 code requiring it.
>
> Thanks,
> Andrew
>
>>
>> Thanks,
>>
>> -Rong
>>
>> Index: gcc.c
>> ===
>> --- gcc.c (revision 205053)
>> +++ gcc.c (working copy)
>> @@ -748,6 +748,23 @@ proper position among the other output files. */
>> %{fvtable-verify=preinit: -lvtv -u_vtable_map_vars_start
>> -u_vtable_map_vars_end}}"
>> #endif
>>
>> +/* This spec is for linking in libatomic in gcov atomic counter update.
>> + We will use the atomic functions defined in libatomic, only when the
>> builtin
>> + versions are not available. In the case of no LD_AS_NEEDED support, we
>> + link libatomic statically. */
>> +
>> +#ifndef GCOV_ATOMIC_SPEC
>> +#if USE_LD_AS_NEEDED
>> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:"
>> LD_AS_NEEDED_OPTION \
>> + " -latomic} " LD_NO_AS_NEEDED_OPTION
>> +#elif defined(HAVE_LD_STATIC_DYNAMIC)
>> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:" LD_STATIC_OPTION \
>> +" -latomic " LD_DYNAMIC_OPTION "}"
>> +#else /* !USE_LD_AS_NEEDED && !HAVE_LD_STATIC_DYNAMIC */
>> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:-latomic}"
>> +#endif
>> +#endif
>> +
>> /* -u* was put back because both BSD and SysV seem to support it. */
>> /* %{static:} simply prevents an error message if the target machine
>> doesn't handle -static. */
>> @@ -771,7 +788,8 @@ proper position among the other output files. */
>>
>> %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
>> %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
>> %(mflib) " STACK_SPLIT_SPEC "\
>> -%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
>> +%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
>> + " GCOV_ATOMIC_SPEC "} " SANITIZER_SPEC " \
>> %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
>> %{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
>>
>>
>>
>> On Wed, Nov 20, 2013 at 1:04 PM, Joseph S. Myers
>> wrote:
>>> On Wed, 20 Nov 2013, Rong Xu wrote:
>>>
I could do this in the SPEC
-Wl,-Bstatic -latomic -Wl,-Bdynamic
which would link libatomic statically.
I works for me. But it looks a little weird in gcc driver.
>>>
>>> I think we should generally link libatomic with --as-needed by default on
>>> platforms supporting --as-needed, in line with the general principle that
>>> C code just using language not library facilities (_Atomic in this case)
>>> shouldn't need any special options to link it (libatomic is like libgcc,
>>> which is linked in automatically); the trickier question is what to do
>>> with it on any systems supporting shared libraries but not --as-needed.
>>>
>>> --
>>> Joseph S. Myers
>>> jos...@codesourcery.com
Re: atomic update of profile counters (issue7000044)
On Wed, Nov 20, 2013 at 2:07 PM, Rong Xu wrote:
> Joseph and Andrew, thanks for the suggestion. That's really helpful.
>
> Here is the new patch for gcc.c.
> Basically, it's just what you have suggested: enclosing -latomic with
> --as-needed, and using macros.
> For the case of no --as-needed support, I use static link. (just found
> that some code already using this in the SPEC).
> I'm flexible on this part -- if you think this is unnecessary, I can remove.
I think Joseph's suggestion was also to include -latomic even when not
generating atomic profiling due to the C11 code requiring it.
Thanks,
Andrew
>
> Thanks,
>
> -Rong
>
> Index: gcc.c
> ===
> --- gcc.c (revision 205053)
> +++ gcc.c (working copy)
> @@ -748,6 +748,23 @@ proper position among the other output files. */
> %{fvtable-verify=preinit: -lvtv -u_vtable_map_vars_start
> -u_vtable_map_vars_end}}"
> #endif
>
> +/* This spec is for linking in libatomic in gcov atomic counter update.
> + We will use the atomic functions defined in libatomic, only when the
> builtin
> + versions are not available. In the case of no LD_AS_NEEDED support, we
> + link libatomic statically. */
> +
> +#ifndef GCOV_ATOMIC_SPEC
> +#if USE_LD_AS_NEEDED
> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:" LD_AS_NEEDED_OPTION
> \
> + " -latomic} " LD_NO_AS_NEEDED_OPTION
> +#elif defined(HAVE_LD_STATIC_DYNAMIC)
> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:" LD_STATIC_OPTION \
> +" -latomic " LD_DYNAMIC_OPTION "}"
> +#else /* !USE_LD_AS_NEEDED && !HAVE_LD_STATIC_DYNAMIC */
> +#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:-latomic}"
> +#endif
> +#endif
> +
> /* -u* was put back because both BSD and SysV seem to support it. */
> /* %{static:} simply prevents an error message if the target machine
> doesn't handle -static. */
> @@ -771,7 +788,8 @@ proper position among the other output files. */
> %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
> %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
> %(mflib) " STACK_SPLIT_SPEC "\
> -%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
> +%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
> + " GCOV_ATOMIC_SPEC "} " SANITIZER_SPEC " \
> %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
> %{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
>
>
>
> On Wed, Nov 20, 2013 at 1:04 PM, Joseph S. Myers
> wrote:
>> On Wed, 20 Nov 2013, Rong Xu wrote:
>>
>>> I could do this in the SPEC
>>> -Wl,-Bstatic -latomic -Wl,-Bdynamic
>>> which would link libatomic statically.
>>> I works for me. But it looks a little weird in gcc driver.
>>
>> I think we should generally link libatomic with --as-needed by default on
>> platforms supporting --as-needed, in line with the general principle that
>> C code just using language not library facilities (_Atomic in this case)
>> shouldn't need any special options to link it (libatomic is like libgcc,
>> which is linked in automatically); the trickier question is what to do
>> with it on any systems supporting shared libraries but not --as-needed.
>>
>> --
>> Joseph S. Myers
>> jos...@codesourcery.com
Re: atomic update of profile counters (issue7000044)
Joseph and Andrew, thanks for the suggestion. That's really helpful.
Here is the new patch for gcc.c.
Basically, it's just what you have suggested: enclosing -latomic with
--as-needed, and using macros.
For the case of no --as-needed support, I use static link. (just found
that some code already using this in the SPEC).
I'm flexible on this part -- if you think this is unnecessary, I can remove.
Thanks,
-Rong
Index: gcc.c
===
--- gcc.c (revision 205053)
+++ gcc.c (working copy)
@@ -748,6 +748,23 @@ proper position among the other output files. */
%{fvtable-verify=preinit: -lvtv -u_vtable_map_vars_start
-u_vtable_map_vars_end}}"
#endif
+/* This spec is for linking in libatomic in gcov atomic counter update.
+ We will use the atomic functions defined in libatomic, only when the builtin
+ versions are not available. In the case of no LD_AS_NEEDED support, we
+ link libatomic statically. */
+
+#ifndef GCOV_ATOMIC_SPEC
+#if USE_LD_AS_NEEDED
+#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:" LD_AS_NEEDED_OPTION \
+ " -latomic} " LD_NO_AS_NEEDED_OPTION
+#elif defined(HAVE_LD_STATIC_DYNAMIC)
+#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:" LD_STATIC_OPTION \
+" -latomic " LD_DYNAMIC_OPTION "}"
+#else /* !USE_LD_AS_NEEDED && !HAVE_LD_STATIC_DYNAMIC */
+#define GCOV_ATOMIC_SPEC "%{fprofile-generate-atomic=*:-latomic}"
+#endif
+#endif
+
/* -u* was put back because both BSD and SysV seem to support it. */
/* %{static:} simply prevents an error message if the target machine
doesn't handle -static. */
@@ -771,7 +788,8 @@ proper position among the other output files. */
%{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
%{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
%(mflib) " STACK_SPLIT_SPEC "\
-%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
+%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
+ " GCOV_ATOMIC_SPEC "} " SANITIZER_SPEC " \
%{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
%{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
On Wed, Nov 20, 2013 at 1:04 PM, Joseph S. Myers
wrote:
> On Wed, 20 Nov 2013, Rong Xu wrote:
>
>> I could do this in the SPEC
>> -Wl,-Bstatic -latomic -Wl,-Bdynamic
>> which would link libatomic statically.
>> I works for me. But it looks a little weird in gcc driver.
>
> I think we should generally link libatomic with --as-needed by default on
> platforms supporting --as-needed, in line with the general principle that
> C code just using language not library facilities (_Atomic in this case)
> shouldn't need any special options to link it (libatomic is like libgcc,
> which is linked in automatically); the trickier question is what to do
> with it on any systems supporting shared libraries but not --as-needed.
>
> --
> Joseph S. Myers
> jos...@codesourcery.com
Re: atomic update of profile counters (issue7000044)
On Wed, 20 Nov 2013, Rong Xu wrote: > I could do this in the SPEC > -Wl,-Bstatic -latomic -Wl,-Bdynamic > which would link libatomic statically. > I works for me. But it looks a little weird in gcc driver. I think we should generally link libatomic with --as-needed by default on platforms supporting --as-needed, in line with the general principle that C code just using language not library facilities (_Atomic in this case) shouldn't need any special options to link it (libatomic is like libgcc, which is linked in automatically); the trickier question is what to do with it on any systems supporting shared libraries but not --as-needed. -- Joseph S. Myers jos...@codesourcery.com
Re: atomic update of profile counters (issue7000044)
On Wed, Nov 20, 2013 at 10:48 AM, Andrew Pinski wrote:
> On Wed, Nov 20, 2013 at 10:44 AM, Rong Xu wrote:
>> On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski wrote:
>>> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu wrote:
Hi all,
I merged this old patch with current trunk. I also make the following
changes
(1) not using weak references. Now every *profile_atomic() has it's
own .o so that none of them will be in the final binary if
-fprofile-generate-atomic is not specified.
(2) more value profilers have the atomic version.
(3) not link to libatomic. I used to link the libatomic in the
presence of -fprofile-generate-atomic, per Andrew's suggestion. It
used to work. But now if I can add -latomic in the SPEC, it cannot
find the libatomic.so.1 (unless I specify the PATH). I did not find an
easy way to statically link libatomic.a. Andrew: Do you have any
suggestion? Or should we let the user link to libatomic.a if the
builtins are not expanded?
>>>
>>> It should work for an installed GCC. For testing you might need
>>> something that is included inside testsuite/lib/atomic-dg.exp which
>>> sets the library path to include libatomic build directory.
>>
>> When I change the SPEC to include libatomic,
>> the compiler can find libatomic. I.e. using
gcc -O2 -fprofile-generate -fprofile-generate-atomic=3 hello_world.c
>> generates a.out without any problem.
>>
>> But since there are both shared and static libatomic in lib64, it
>> chooses to use the dynamic one.
ldd a.out
>> linux-vdso.so.1 => (0x7fff56bff000)
>> libatomic.so.1 => not found
>> libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x2b0720261000)
>> /lib64/ld-linux-x86-64.so.2 (0x2b072003c000)
>>
./a.out
>> ./a.out: error while loading shared libraries: libatomic.so.1: cannot
>> open shared object file: No such file or directory
>>
>> while
LD_LIBRARY_PATH=/lib64 ./a.out
>> works fine.
>
>
> I don't see this as an issue really as you have the same issue with
> all the target libraries (not limited to libatomic or libgomp or
> libgfortran).
You also also use --as-needed/--no-as-needed wrapped around the
-latomic too; USE_LD_AS_NEEDED is needed to be used to check for
--as-needed support and LD_AS_NEEDED_OPTION/LD_NO_AS_NEEDED_OPTION
should be used instead of directly --as-needed/--no-as-needed.
>
> Thanks,
> Andrew Pinski
>
>>
>> I think that's the same reason we set the library path in
>> testsuite/lib/atomic-dg.exp, because /lib64
>> is not in the dynamic library search list.
>>
>> I could do this in the SPEC
>> -Wl,-Bstatic -latomic -Wl,-Bdynamic
>> which would link libatomic statically.
>> I works for me. But it looks a little weird in gcc driver.
>>
>> Index: gcc.c
>> ===
>> --- gcc.c (revision 205053)
>> +++ gcc.c (working copy)
>> @@ -771,7 +771,8 @@
>>
>> %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
>> %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
>> %(mflib) " STACK_SPLIT_SPEC "\
>> -%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
>> +%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
>> + %{fprofile-generate-atomic=*:-Wl,-Bstatic -latomic
>> -Wl,-Bdynamic}} " SANITIZER_SPEC " \
>> %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
>> %{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
>> #endif
>>
>>
>>
>>> I think now we require libatomic in more cases (C11 atomic support for
>>> an example).
>>>
>>> Thanks,
>>> Andrew Pinski
>>>
Is this OK for trunk?
Thanks,
-Rong
On Mon, Jan 7, 2013 at 12:55 PM, Rong Xu wrote:
> Function __gcov_indirect_call_profiler_atomic (which contains call to
> the atomic function) is always emitted in libgcov.
> Since we only link libatomic when -fprofile-gen-atomic is specified,
> we have to make the atomic function weak -- otherwise, there is a
> unsat for regular FDO gen build (of course, when the builtin is not
> expanded).
>
> An alternative it to always link libatomic together with libgcov. Then
> we don't need the weak stuff. I'm not sure when one is better.
>
> -Rong
>
> On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson
> wrote:
>> On 01/03/2013 04:42 PM, Rong Xu wrote:
>>> It links libatomic when -fprofile-gen-atomic is specified for FDO
>>> instrumentation build. Here I assume libatomic is always installed.
>>> Andrew: do you think if this is reasonable?
>>>
>>> It also disables the functionality if target does not support weak
>>> (ie. TARGET_SUPPORTS_WEAK == 0).
>>
>> Since you're linking libatomic, you don't need weak references.
>>
>> I think its ok to assume libatomic is installed, given that the
>> user has had to explicitly use the command-line option.
>>
>>
>> r
Re: atomic update of profile counters (issue7000044)
On Wed, Nov 20, 2013 at 10:44 AM, Rong Xu wrote:
> On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski wrote:
>> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu wrote:
>>> Hi all,
>>>
>>> I merged this old patch with current trunk. I also make the following
>>> changes
>>> (1) not using weak references. Now every *profile_atomic() has it's
>>> own .o so that none of them will be in the final binary if
>>> -fprofile-generate-atomic is not specified.
>>> (2) more value profilers have the atomic version.
>>> (3) not link to libatomic. I used to link the libatomic in the
>>> presence of -fprofile-generate-atomic, per Andrew's suggestion. It
>>> used to work. But now if I can add -latomic in the SPEC, it cannot
>>> find the libatomic.so.1 (unless I specify the PATH). I did not find an
>>> easy way to statically link libatomic.a. Andrew: Do you have any
>>> suggestion? Or should we let the user link to libatomic.a if the
>>> builtins are not expanded?
>>
>> It should work for an installed GCC. For testing you might need
>> something that is included inside testsuite/lib/atomic-dg.exp which
>> sets the library path to include libatomic build directory.
>
> When I change the SPEC to include libatomic,
> the compiler can find libatomic. I.e. using
>>> gcc -O2 -fprofile-generate -fprofile-generate-atomic=3 hello_world.c
> generates a.out without any problem.
>
> But since there are both shared and static libatomic in lib64, it
> chooses to use the dynamic one.
>>> ldd a.out
> linux-vdso.so.1 => (0x7fff56bff000)
> libatomic.so.1 => not found
> libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x2b0720261000)
> /lib64/ld-linux-x86-64.so.2 (0x2b072003c000)
>
>>> ./a.out
> ./a.out: error while loading shared libraries: libatomic.so.1: cannot
> open shared object file: No such file or directory
>
> while
>>> LD_LIBRARY_PATH=/lib64 ./a.out
> works fine.
I don't see this as an issue really as you have the same issue with
all the target libraries (not limited to libatomic or libgomp or
libgfortran).
Thanks,
Andrew Pinski
>
> I think that's the same reason we set the library path in
> testsuite/lib/atomic-dg.exp, because /lib64
> is not in the dynamic library search list.
>
> I could do this in the SPEC
> -Wl,-Bstatic -latomic -Wl,-Bdynamic
> which would link libatomic statically.
> I works for me. But it looks a little weird in gcc driver.
>
> Index: gcc.c
> ===
> --- gcc.c (revision 205053)
> +++ gcc.c (working copy)
> @@ -771,7 +771,8 @@
> %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
> %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
> %(mflib) " STACK_SPLIT_SPEC "\
> -%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
> +%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
> + %{fprofile-generate-atomic=*:-Wl,-Bstatic -latomic
> -Wl,-Bdynamic}} " SANITIZER_SPEC " \
> %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
> %{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
> #endif
>
>
>
>> I think now we require libatomic in more cases (C11 atomic support for
>> an example).
>>
>> Thanks,
>> Andrew Pinski
>>
>>>
>>> Is this OK for trunk?
>>>
>>> Thanks,
>>>
>>> -Rong
>>>
>>> On Mon, Jan 7, 2013 at 12:55 PM, Rong Xu wrote:
Function __gcov_indirect_call_profiler_atomic (which contains call to
the atomic function) is always emitted in libgcov.
Since we only link libatomic when -fprofile-gen-atomic is specified,
we have to make the atomic function weak -- otherwise, there is a
unsat for regular FDO gen build (of course, when the builtin is not
expanded).
An alternative it to always link libatomic together with libgcov. Then
we don't need the weak stuff. I'm not sure when one is better.
-Rong
On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson wrote:
> On 01/03/2013 04:42 PM, Rong Xu wrote:
>> It links libatomic when -fprofile-gen-atomic is specified for FDO
>> instrumentation build. Here I assume libatomic is always installed.
>> Andrew: do you think if this is reasonable?
>>
>> It also disables the functionality if target does not support weak
>> (ie. TARGET_SUPPORTS_WEAK == 0).
>
> Since you're linking libatomic, you don't need weak references.
>
> I think its ok to assume libatomic is installed, given that the
> user has had to explicitly use the command-line option.
>
>
> r~
Re: atomic update of profile counters (issue7000044)
On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski wrote:
> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu wrote:
>> Hi all,
>>
>> I merged this old patch with current trunk. I also make the following changes
>> (1) not using weak references. Now every *profile_atomic() has it's
>> own .o so that none of them will be in the final binary if
>> -fprofile-generate-atomic is not specified.
>> (2) more value profilers have the atomic version.
>> (3) not link to libatomic. I used to link the libatomic in the
>> presence of -fprofile-generate-atomic, per Andrew's suggestion. It
>> used to work. But now if I can add -latomic in the SPEC, it cannot
>> find the libatomic.so.1 (unless I specify the PATH). I did not find an
>> easy way to statically link libatomic.a. Andrew: Do you have any
>> suggestion? Or should we let the user link to libatomic.a if the
>> builtins are not expanded?
>
> It should work for an installed GCC. For testing you might need
> something that is included inside testsuite/lib/atomic-dg.exp which
> sets the library path to include libatomic build directory.
When I change the SPEC to include libatomic,
the compiler can find libatomic. I.e. using
>> gcc -O2 -fprofile-generate -fprofile-generate-atomic=3 hello_world.c
generates a.out without any problem.
But since there are both shared and static libatomic in lib64, it
chooses to use the dynamic one.
>> ldd a.out
linux-vdso.so.1 => (0x7fff56bff000)
libatomic.so.1 => not found
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x2b0720261000)
/lib64/ld-linux-x86-64.so.2 (0x2b072003c000)
>> ./a.out
./a.out: error while loading shared libraries: libatomic.so.1: cannot
open shared object file: No such file or directory
while
>> LD_LIBRARY_PATH=/lib64 ./a.out
works fine.
I think that's the same reason we set the library path in
testsuite/lib/atomic-dg.exp, because /lib64
is not in the dynamic library search list.
I could do this in the SPEC
-Wl,-Bstatic -latomic -Wl,-Bdynamic
which would link libatomic statically.
I works for me. But it looks a little weird in gcc driver.
Index: gcc.c
===
--- gcc.c (revision 205053)
+++ gcc.c (working copy)
@@ -771,7 +771,8 @@
%{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
%{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
%(mflib) " STACK_SPLIT_SPEC "\
-%{fprofile-arcs|fprofile-generate*|coverage:-lgcov} " SANITIZER_SPEC " \
+%{fprofile-arcs|fprofile-generate*|coverage:-lgcov\
+ %{fprofile-generate-atomic=*:-Wl,-Bstatic -latomic
-Wl,-Bdynamic}} " SANITIZER_SPEC " \
%{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
%{!nostdlib:%{!nostartfiles:%E}} %{T*} }}"
#endif
> I think now we require libatomic in more cases (C11 atomic support for
> an example).
>
> Thanks,
> Andrew Pinski
>
>>
>> Is this OK for trunk?
>>
>> Thanks,
>>
>> -Rong
>>
>> On Mon, Jan 7, 2013 at 12:55 PM, Rong Xu wrote:
>>> Function __gcov_indirect_call_profiler_atomic (which contains call to
>>> the atomic function) is always emitted in libgcov.
>>> Since we only link libatomic when -fprofile-gen-atomic is specified,
>>> we have to make the atomic function weak -- otherwise, there is a
>>> unsat for regular FDO gen build (of course, when the builtin is not
>>> expanded).
>>>
>>> An alternative it to always link libatomic together with libgcov. Then
>>> we don't need the weak stuff. I'm not sure when one is better.
>>>
>>> -Rong
>>>
>>> On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson wrote:
On 01/03/2013 04:42 PM, Rong Xu wrote:
> It links libatomic when -fprofile-gen-atomic is specified for FDO
> instrumentation build. Here I assume libatomic is always installed.
> Andrew: do you think if this is reasonable?
>
> It also disables the functionality if target does not support weak
> (ie. TARGET_SUPPORTS_WEAK == 0).
Since you're linking libatomic, you don't need weak references.
I think its ok to assume libatomic is installed, given that the
user has had to explicitly use the command-line option.
r~
Re: atomic update of profile counters (issue7000044)
On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu wrote: > Hi all, > > I merged this old patch with current trunk. I also make the following changes > (1) not using weak references. Now every *profile_atomic() has it's > own .o so that none of them will be in the final binary if > -fprofile-generate-atomic is not specified. > (2) more value profilers have the atomic version. > (3) not link to libatomic. I used to link the libatomic in the > presence of -fprofile-generate-atomic, per Andrew's suggestion. It > used to work. But now if I can add -latomic in the SPEC, it cannot > find the libatomic.so.1 (unless I specify the PATH). I did not find an > easy way to statically link libatomic.a. Andrew: Do you have any > suggestion? Or should we let the user link to libatomic.a if the > builtins are not expanded? It should work for an installed GCC. For testing you might need something that is included inside testsuite/lib/atomic-dg.exp which sets the library path to include libatomic build directory. I think now we require libatomic in more cases (C11 atomic support for an example). Thanks, Andrew Pinski > > Is this OK for trunk? > > Thanks, > > -Rong > > On Mon, Jan 7, 2013 at 12:55 PM, Rong Xu wrote: >> Function __gcov_indirect_call_profiler_atomic (which contains call to >> the atomic function) is always emitted in libgcov. >> Since we only link libatomic when -fprofile-gen-atomic is specified, >> we have to make the atomic function weak -- otherwise, there is a >> unsat for regular FDO gen build (of course, when the builtin is not >> expanded). >> >> An alternative it to always link libatomic together with libgcov. Then >> we don't need the weak stuff. I'm not sure when one is better. >> >> -Rong >> >> On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson wrote: >>> On 01/03/2013 04:42 PM, Rong Xu wrote: It links libatomic when -fprofile-gen-atomic is specified for FDO instrumentation build. Here I assume libatomic is always installed. Andrew: do you think if this is reasonable? It also disables the functionality if target does not support weak (ie. TARGET_SUPPORTS_WEAK == 0). >>> >>> Since you're linking libatomic, you don't need weak references. >>> >>> I think its ok to assume libatomic is installed, given that the >>> user has had to explicitly use the command-line option. >>> >>> >>> r~
Re: atomic update of profile counters (issue7000044)
Hi all,
I merged this old patch with current trunk. I also make the following changes
(1) not using weak references. Now every *profile_atomic() has it's
own .o so that none of them will be in the final binary if
-fprofile-generate-atomic is not specified.
(2) more value profilers have the atomic version.
(3) not link to libatomic. I used to link the libatomic in the
presence of -fprofile-generate-atomic, per Andrew's suggestion. It
used to work. But now if I can add -latomic in the SPEC, it cannot
find the libatomic.so.1 (unless I specify the PATH). I did not find an
easy way to statically link libatomic.a. Andrew: Do you have any
suggestion? Or should we let the user link to libatomic.a if the
builtins are not expanded?
Is this OK for trunk?
Thanks,
-Rong
On Mon, Jan 7, 2013 at 12:55 PM, Rong Xu wrote:
> Function __gcov_indirect_call_profiler_atomic (which contains call to
> the atomic function) is always emitted in libgcov.
> Since we only link libatomic when -fprofile-gen-atomic is specified,
> we have to make the atomic function weak -- otherwise, there is a
> unsat for regular FDO gen build (of course, when the builtin is not
> expanded).
>
> An alternative it to always link libatomic together with libgcov. Then
> we don't need the weak stuff. I'm not sure when one is better.
>
> -Rong
>
> On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson wrote:
>> On 01/03/2013 04:42 PM, Rong Xu wrote:
>>> It links libatomic when -fprofile-gen-atomic is specified for FDO
>>> instrumentation build. Here I assume libatomic is always installed.
>>> Andrew: do you think if this is reasonable?
>>>
>>> It also disables the functionality if target does not support weak
>>> (ie. TARGET_SUPPORTS_WEAK == 0).
>>
>> Since you're linking libatomic, you don't need weak references.
>>
>> I think its ok to assume libatomic is installed, given that the
>> user has had to explicitly use the command-line option.
>>
>>
>> r~
2013-11-19 Rong Xu
* gcc/gcov-io.h: Add atomic function macros for compiler use.
* gcc/common.opt (fprofile-generate-atomic): New option.
* gcc/tree-profile.c (gimple_init_edge_profiler): Support for
atomic counter update.
(gimple_gen_edge_profiler): Ditto.
* libgcc/libgcov-profiler.c
(__gcov_interval_profiler_atomic): Ditto.
(__gcov_pow2_profiler_atomic): Ditto.
(__gcov_one_value_profiler_body_atomic): Ditto.
(__gcov_one_value_profiler_atomic): Ditto.
(__gcov_indirect_call_profiler_atomic): Ditto.
(__gcov_indirect_call_profiler_v2_atomic): Ditto.
(__gcov_time_profiler_atomic): Ditto.
(__gcov_average_profiler_atomic): Ditto.
* gcc/gcc.c: Link libatomic when -fprofile-generate-atomic used.
* libgcc/Makefile.in: Add atomic objects.
Index: gcc/common.opt
===
--- gcc/common.opt (revision 205053)
+++ gcc/common.opt (working copy)
@@ -1684,6 +1684,15 @@ fprofile-correction
Common Report Var(flag_profile_correction)
Enable correction of flow inconsistent profile data input
+; fprofile-generate-atomic=0: default and disable atomical update.
+; fprofile-generate-atomic=1: atomically update edge profile counters.
+; fprofile-generate-atomic=2: atomically update value profile counters.
+; fprofile-generate-atomic=3: atomically update edge and value profile
counters.
+; other values will be ignored (fall back to the default of 0).
+fprofile-generate-atomic=
+Common Joined UInteger Report Var(flag_profile_generate_atomic) Init(3)
Optimization
+fprofile-generate-atomic=[0..3] Atomical increments of profile counters.
+
fprofile-generate
Common
Enable common options for generating profile info for profile feedback
directed optimizations
Index: gcc/tree-profile.c
===
--- gcc/tree-profile.c (revision 205053)
+++ gcc/tree-profile.c (working copy)
@@ -155,6 +155,9 @@ gimple_init_edge_profiler (void)
tree ic_profiler_fn_type;
tree average_profiler_fn_type;
tree time_profiler_fn_type;
+ const char *fn_name;
+ bool profile_gen_value_atomic = (flag_profile_generate_atomic == 2 ||
+ flag_profile_generate_atomic == 3);
if (!gcov_type_node)
{
@@ -167,9 +170,10 @@ gimple_init_edge_profiler (void)
gcov_type_ptr, gcov_type_node,
integer_type_node,
unsigned_type_node, NULL_TREE);
+ fn_name = profile_gen_value_atomic ? "__gcov_interval_profiler_atomic" :
+ "__gcov_interval_profiler";
tree_interval_profiler_fn
- = build_fn_decl ("__gcov_interval_profiler",
-interval_profiler_fn_type);
+ = build_fn_decl (fn_name, interval_profiler_fn_type);
TREE_NOTHROW (tree_interval_profiler_fn) = 1;
Re: atomic update of profile counters (issue7000044)
Function __gcov_indirect_call_profiler_atomic (which contains call to the atomic function) is always emitted in libgcov. Since we only link libatomic when -fprofile-gen-atomic is specified, we have to make the atomic function weak -- otherwise, there is a unsat for regular FDO gen build (of course, when the builtin is not expanded). An alternative it to always link libatomic together with libgcov. Then we don't need the weak stuff. I'm not sure when one is better. -Rong On Mon, Jan 7, 2013 at 12:36 PM, Richard Henderson wrote: > On 01/03/2013 04:42 PM, Rong Xu wrote: >> It links libatomic when -fprofile-gen-atomic is specified for FDO >> instrumentation build. Here I assume libatomic is always installed. >> Andrew: do you think if this is reasonable? >> >> It also disables the functionality if target does not support weak >> (ie. TARGET_SUPPORTS_WEAK == 0). > > Since you're linking libatomic, you don't need weak references. > > I think its ok to assume libatomic is installed, given that the > user has had to explicitly use the command-line option. > > > r~
Re: atomic update of profile counters (issue7000044)
On 01/03/2013 04:42 PM, Rong Xu wrote: > It links libatomic when -fprofile-gen-atomic is specified for FDO > instrumentation build. Here I assume libatomic is always installed. > Andrew: do you think if this is reasonable? > > It also disables the functionality if target does not support weak > (ie. TARGET_SUPPORTS_WEAK == 0). Since you're linking libatomic, you don't need weak references. I think its ok to assume libatomic is installed, given that the user has had to explicitly use the command-line option. r~
Re: atomic update of profile counters (issue7000044)
Here is the new patch. It links libatomic when -fprofile-gen-atomic is specified for FDO instrumentation build. Here I assume libatomic is always installed. Andrew: do you think if this is reasonable? It also disables the functionality if target does not support weak (ie. TARGET_SUPPORTS_WEAK == 0). Thanks, -Rong On Thu, Jan 3, 2013 at 1:05 AM, Richard Biener wrote: > On Thu, Jan 3, 2013 at 2:25 AM, Andrew Pinski wrote: >> On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu wrote: >>> Hi, >>> >>> Here is a new patch. The only difference is to declare >>> __atomic_fetch_add as weak. This is >>> needed for targets without sync/atomic builtin support. The patch >>> contains a call to the builtin regardless of the new options >>> -fprofile-gen-atomic. This results in a unsat in these targets even >>> for regular profile-gen built. >>> >>> With this new patch, if the user uses -fprofile-gen-atomic in these >>> target, the generated code will seg fault. >>> >>> We think a better solution is to emit the builtin call only in these >>> targets with the support, and give warning for non-supported target. >>> But I did not find any target hook for this. Does anyone know how to >>> do this? >> >> Why not use libatomic for those targets? > > Also note that not all targets support 'weak' linkage. How about check the flag TARGET_SUPPORTS_WEAK, and only enable the code when the flag is true. > > Richard. > >> Thanks, >> Andrew Pinski >> >> >> >>> >>> Thanks, >>> >>> -Rong >>> >>> >>> On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li >>> wrote: It would be great if this can make into gcc4.8. The patch has close to 0 impact on code stability. David On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: > Hi Honza, > > In the other thread of discussion (similar patch in google-4_7 > branch), you said you were thinking if to let this patch into trunk in > stage 3. Can you give some update? > > Thanks, > > -Rong > > On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: >> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: Hi, This patch adds support of atomic update of profiles counters. The goal is to improve the poor counter values for highly thread programs. The atomic update is under a new option -fprofile-gen-atomic= N=0: default, no atomic update N=1: atomic update edge counters. N=2: atomic update some of value profile counters (currently indirect-call and one value profile). N=3: both edge counter and the above value profile counters. Other value: fall back to the default. This patch is a simple porting of the version in google-4_7 branch. It uses __atomic_fetch_add based on Andrew Pinski's suggestion. Note I did not apply to all the value profiles as the indirect-call profile is the most relevant one here. Test with bootstrap. Comments and suggestions are welcomed. Thanks, -Rong 2012-12-20 Rong Xu * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New function. Atomic update profile counters. (__gcov_one_value_profiler_atomic): Ditto. (__gcov_indirect_call_profiler_atomic): Ditto. * gcc/gcov-io.h: Macros for atomic update. * gcc/common.opt: New option. * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic update profile counters. (gimple_gen_edge_profiler): Ditto. >>> >>> The patch looks resonable. Eventually we probably should provide rest >>> of the value counters >>> in thread safe manner. What happens on targets not having atomic >>> operations? >> >> From >> http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, >> it says: >> "If a particular operation cannot be implemented on the target >> processor, a warning is generated and a call an external function is >> generated. " >> >> So I think there will be a warning and eventually a link error of unsat. >> >> Thanks, >> >> -Rong >> >> >>> >>> Honza patch3.diff Description: Binary data
Re: atomic update of profile counters (issue7000044)
On Thu, Jan 3, 2013 at 2:25 AM, Andrew Pinski wrote: > On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu wrote: >> Hi, >> >> Here is a new patch. The only difference is to declare >> __atomic_fetch_add as weak. This is >> needed for targets without sync/atomic builtin support. The patch >> contains a call to the builtin regardless of the new options >> -fprofile-gen-atomic. This results in a unsat in these targets even >> for regular profile-gen built. >> >> With this new patch, if the user uses -fprofile-gen-atomic in these >> target, the generated code will seg fault. >> >> We think a better solution is to emit the builtin call only in these >> targets with the support, and give warning for non-supported target. >> But I did not find any target hook for this. Does anyone know how to >> do this? > > Why not use libatomic for those targets? Also note that not all targets support 'weak' linkage. Richard. > Thanks, > Andrew Pinski > > > >> >> Thanks, >> >> -Rong >> >> >> On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li >> wrote: >>> It would be great if this can make into gcc4.8. The patch has close to >>> 0 impact on code stability. >>> >>> David >>> >>> On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: Hi Honza, In the other thread of discussion (similar patch in google-4_7 branch), you said you were thinking if to let this patch into trunk in stage 3. Can you give some update? Thanks, -Rong On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: > On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: >>> Hi, >>> >>> This patch adds support of atomic update of profiles counters. The goal >>> is to improve >>> the poor counter values for highly thread programs. >>> >>> The atomic update is under a new option -fprofile-gen-atomic= >>> N=0: default, no atomic update >>> N=1: atomic update edge counters. >>> N=2: atomic update some of value profile counters (currently >>> indirect-call and one value profile). >>> N=3: both edge counter and the above value profile counters. >>> Other value: fall back to the default. >>> >>> This patch is a simple porting of the version in google-4_7 branch. It >>> uses __atomic_fetch_add >>> based on Andrew Pinski's suggestion. Note I did not apply to all the >>> value profiles as >>> the indirect-call profile is the most relevant one here. >>> >>> Test with bootstrap. >>> >>> Comments and suggestions are welcomed. >>> >>> Thanks, >>> >>> -Rong >>> >>> >>> 2012-12-20 Rong Xu >>> >>> * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New >>> function. Atomic update profile counters. >>> (__gcov_one_value_profiler_atomic): Ditto. >>> (__gcov_indirect_call_profiler_atomic): Ditto. >>> * gcc/gcov-io.h: Macros for atomic update. >>> * gcc/common.opt: New option. >>> * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic >>> update profile counters. >>> (gimple_gen_edge_profiler): Ditto. >> >> The patch looks resonable. Eventually we probably should provide rest >> of the value counters >> in thread safe manner. What happens on targets not having atomic >> operations? > > From > http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, > it says: > "If a particular operation cannot be implemented on the target > processor, a warning is generated and a call an external function is > generated. " > > So I think there will be a warning and eventually a link error of unsat. > > Thanks, > > -Rong > > >> >> Honza
Re: atomic update of profile counters (issue7000044)
On Wed, Jan 2, 2013 at 5:29 PM, Rong Xu wrote: > Does libatomic support all targets? It supports all targets that support pthreads. Thanks, Andrew > I think it's a good idea to change the driver to link in this library > if the option is specified. > But still, we need to make the builtin weak. > > Thanks, > > -Rong > > On Wed, Jan 2, 2013 at 5:25 PM, Andrew Pinski wrote: >> On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu wrote: >>> Hi, >>> >>> Here is a new patch. The only difference is to declare >>> __atomic_fetch_add as weak. This is >>> needed for targets without sync/atomic builtin support. The patch >>> contains a call to the builtin regardless of the new options >>> -fprofile-gen-atomic. This results in a unsat in these targets even >>> for regular profile-gen built. >>> >>> With this new patch, if the user uses -fprofile-gen-atomic in these >>> target, the generated code will seg fault. >>> >>> We think a better solution is to emit the builtin call only in these >>> targets with the support, and give warning for non-supported target. >>> But I did not find any target hook for this. Does anyone know how to >>> do this? >> >> Why not use libatomic for those targets? >> >> Thanks, >> Andrew Pinski >> >> >> >>> >>> Thanks, >>> >>> -Rong >>> >>> >>> On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li >>> wrote: It would be great if this can make into gcc4.8. The patch has close to 0 impact on code stability. David On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: > Hi Honza, > > In the other thread of discussion (similar patch in google-4_7 > branch), you said you were thinking if to let this patch into trunk in > stage 3. Can you give some update? > > Thanks, > > -Rong > > On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: >> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: Hi, This patch adds support of atomic update of profiles counters. The goal is to improve the poor counter values for highly thread programs. The atomic update is under a new option -fprofile-gen-atomic= N=0: default, no atomic update N=1: atomic update edge counters. N=2: atomic update some of value profile counters (currently indirect-call and one value profile). N=3: both edge counter and the above value profile counters. Other value: fall back to the default. This patch is a simple porting of the version in google-4_7 branch. It uses __atomic_fetch_add based on Andrew Pinski's suggestion. Note I did not apply to all the value profiles as the indirect-call profile is the most relevant one here. Test with bootstrap. Comments and suggestions are welcomed. Thanks, -Rong 2012-12-20 Rong Xu * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New function. Atomic update profile counters. (__gcov_one_value_profiler_atomic): Ditto. (__gcov_indirect_call_profiler_atomic): Ditto. * gcc/gcov-io.h: Macros for atomic update. * gcc/common.opt: New option. * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic update profile counters. (gimple_gen_edge_profiler): Ditto. >>> >>> The patch looks resonable. Eventually we probably should provide rest >>> of the value counters >>> in thread safe manner. What happens on targets not having atomic >>> operations? >> >> From >> http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, >> it says: >> "If a particular operation cannot be implemented on the target >> processor, a warning is generated and a call an external function is >> generated. " >> >> So I think there will be a warning and eventually a link error of unsat. >> >> Thanks, >> >> -Rong >> >> >>> >>> Honza
Re: atomic update of profile counters (issue7000044)
Does libatomic support all targets? I think it's a good idea to change the driver to link in this library if the option is specified. But still, we need to make the builtin weak. Thanks, -Rong On Wed, Jan 2, 2013 at 5:25 PM, Andrew Pinski wrote: > On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu wrote: >> Hi, >> >> Here is a new patch. The only difference is to declare >> __atomic_fetch_add as weak. This is >> needed for targets without sync/atomic builtin support. The patch >> contains a call to the builtin regardless of the new options >> -fprofile-gen-atomic. This results in a unsat in these targets even >> for regular profile-gen built. >> >> With this new patch, if the user uses -fprofile-gen-atomic in these >> target, the generated code will seg fault. >> >> We think a better solution is to emit the builtin call only in these >> targets with the support, and give warning for non-supported target. >> But I did not find any target hook for this. Does anyone know how to >> do this? > > Why not use libatomic for those targets? > > Thanks, > Andrew Pinski > > > >> >> Thanks, >> >> -Rong >> >> >> On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li >> wrote: >>> It would be great if this can make into gcc4.8. The patch has close to >>> 0 impact on code stability. >>> >>> David >>> >>> On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: Hi Honza, In the other thread of discussion (similar patch in google-4_7 branch), you said you were thinking if to let this patch into trunk in stage 3. Can you give some update? Thanks, -Rong On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: > On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: >>> Hi, >>> >>> This patch adds support of atomic update of profiles counters. The goal >>> is to improve >>> the poor counter values for highly thread programs. >>> >>> The atomic update is under a new option -fprofile-gen-atomic= >>> N=0: default, no atomic update >>> N=1: atomic update edge counters. >>> N=2: atomic update some of value profile counters (currently >>> indirect-call and one value profile). >>> N=3: both edge counter and the above value profile counters. >>> Other value: fall back to the default. >>> >>> This patch is a simple porting of the version in google-4_7 branch. It >>> uses __atomic_fetch_add >>> based on Andrew Pinski's suggestion. Note I did not apply to all the >>> value profiles as >>> the indirect-call profile is the most relevant one here. >>> >>> Test with bootstrap. >>> >>> Comments and suggestions are welcomed. >>> >>> Thanks, >>> >>> -Rong >>> >>> >>> 2012-12-20 Rong Xu >>> >>> * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New >>> function. Atomic update profile counters. >>> (__gcov_one_value_profiler_atomic): Ditto. >>> (__gcov_indirect_call_profiler_atomic): Ditto. >>> * gcc/gcov-io.h: Macros for atomic update. >>> * gcc/common.opt: New option. >>> * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic >>> update profile counters. >>> (gimple_gen_edge_profiler): Ditto. >> >> The patch looks resonable. Eventually we probably should provide rest >> of the value counters >> in thread safe manner. What happens on targets not having atomic >> operations? > > From > http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, > it says: > "If a particular operation cannot be implemented on the target > processor, a warning is generated and a call an external function is > generated. " > > So I think there will be a warning and eventually a link error of unsat. > > Thanks, > > -Rong > > >> >> Honza
Re: atomic update of profile counters (issue7000044)
On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu wrote: > Hi, > > Here is a new patch. The only difference is to declare > __atomic_fetch_add as weak. This is > needed for targets without sync/atomic builtin support. The patch > contains a call to the builtin regardless of the new options > -fprofile-gen-atomic. This results in a unsat in these targets even > for regular profile-gen built. > > With this new patch, if the user uses -fprofile-gen-atomic in these > target, the generated code will seg fault. > > We think a better solution is to emit the builtin call only in these > targets with the support, and give warning for non-supported target. > But I did not find any target hook for this. Does anyone know how to > do this? Why not use libatomic for those targets? Thanks, Andrew Pinski > > Thanks, > > -Rong > > > On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li > wrote: >> It would be great if this can make into gcc4.8. The patch has close to >> 0 impact on code stability. >> >> David >> >> On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: >>> Hi Honza, >>> >>> In the other thread of discussion (similar patch in google-4_7 >>> branch), you said you were thinking if to let this patch into trunk in >>> stage 3. Can you give some update? >>> >>> Thanks, >>> >>> -Rong >>> >>> On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: >> Hi, >> >> This patch adds support of atomic update of profiles counters. The goal >> is to improve >> the poor counter values for highly thread programs. >> >> The atomic update is under a new option -fprofile-gen-atomic= >> N=0: default, no atomic update >> N=1: atomic update edge counters. >> N=2: atomic update some of value profile counters (currently >> indirect-call and one value profile). >> N=3: both edge counter and the above value profile counters. >> Other value: fall back to the default. >> >> This patch is a simple porting of the version in google-4_7 branch. It >> uses __atomic_fetch_add >> based on Andrew Pinski's suggestion. Note I did not apply to all the >> value profiles as >> the indirect-call profile is the most relevant one here. >> >> Test with bootstrap. >> >> Comments and suggestions are welcomed. >> >> Thanks, >> >> -Rong >> >> >> 2012-12-20 Rong Xu >> >> * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New >> function. Atomic update profile counters. >> (__gcov_one_value_profiler_atomic): Ditto. >> (__gcov_indirect_call_profiler_atomic): Ditto. >> * gcc/gcov-io.h: Macros for atomic update. >> * gcc/common.opt: New option. >> * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic >> update profile counters. >> (gimple_gen_edge_profiler): Ditto. > > The patch looks resonable. Eventually we probably should provide rest of > the value counters > in thread safe manner. What happens on targets not having atomic > operations? From http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, it says: "If a particular operation cannot be implemented on the target processor, a warning is generated and a call an external function is generated. " So I think there will be a warning and eventually a link error of unsat. Thanks, -Rong > > Honza
Re: atomic update of profile counters (issue7000044)
Hi, Here is a new patch. The only difference is to declare __atomic_fetch_add as weak. This is needed for targets without sync/atomic builtin support. The patch contains a call to the builtin regardless of the new options -fprofile-gen-atomic. This results in a unsat in these targets even for regular profile-gen built. With this new patch, if the user uses -fprofile-gen-atomic in these target, the generated code will seg fault. We think a better solution is to emit the builtin call only in these targets with the support, and give warning for non-supported target. But I did not find any target hook for this. Does anyone know how to do this? Thanks, -Rong On Fri, Dec 28, 2012 at 11:35 AM, Xinliang David Li wrote: > It would be great if this can make into gcc4.8. The patch has close to > 0 impact on code stability. > > David > > On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: >> Hi Honza, >> >> In the other thread of discussion (similar patch in google-4_7 >> branch), you said you were thinking if to let this patch into trunk in >> stage 3. Can you give some update? >> >> Thanks, >> >> -Rong >> >> On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: >>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: > Hi, > > This patch adds support of atomic update of profiles counters. The goal > is to improve > the poor counter values for highly thread programs. > > The atomic update is under a new option -fprofile-gen-atomic= > N=0: default, no atomic update > N=1: atomic update edge counters. > N=2: atomic update some of value profile counters (currently > indirect-call and one value profile). > N=3: both edge counter and the above value profile counters. > Other value: fall back to the default. > > This patch is a simple porting of the version in google-4_7 branch. It > uses __atomic_fetch_add > based on Andrew Pinski's suggestion. Note I did not apply to all the > value profiles as > the indirect-call profile is the most relevant one here. > > Test with bootstrap. > > Comments and suggestions are welcomed. > > Thanks, > > -Rong > > > 2012-12-20 Rong Xu > > * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New > function. Atomic update profile counters. > (__gcov_one_value_profiler_atomic): Ditto. > (__gcov_indirect_call_profiler_atomic): Ditto. > * gcc/gcov-io.h: Macros for atomic update. > * gcc/common.opt: New option. > * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic > update profile counters. > (gimple_gen_edge_profiler): Ditto. The patch looks resonable. Eventually we probably should provide rest of the value counters in thread safe manner. What happens on targets not having atomic operations? >>> >>> From >>> http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, >>> it says: >>> "If a particular operation cannot be implemented on the target >>> processor, a warning is generated and a call an external function is >>> generated. " >>> >>> So I think there will be a warning and eventually a link error of unsat. >>> >>> Thanks, >>> >>> -Rong >>> >>> Honza patch2.diff Description: Binary data
Re: atomic update of profile counters (issue7000044)
It would be great if this can make into gcc4.8. The patch has close to 0 impact on code stability. David On Fri, Dec 28, 2012 at 11:32 AM, Rong Xu wrote: > Hi Honza, > > In the other thread of discussion (similar patch in google-4_7 > branch), you said you were thinking if to let this patch into trunk in > stage 3. Can you give some update? > > Thanks, > > -Rong > > On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: >> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: Hi, This patch adds support of atomic update of profiles counters. The goal is to improve the poor counter values for highly thread programs. The atomic update is under a new option -fprofile-gen-atomic= N=0: default, no atomic update N=1: atomic update edge counters. N=2: atomic update some of value profile counters (currently indirect-call and one value profile). N=3: both edge counter and the above value profile counters. Other value: fall back to the default. This patch is a simple porting of the version in google-4_7 branch. It uses __atomic_fetch_add based on Andrew Pinski's suggestion. Note I did not apply to all the value profiles as the indirect-call profile is the most relevant one here. Test with bootstrap. Comments and suggestions are welcomed. Thanks, -Rong 2012-12-20 Rong Xu * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New function. Atomic update profile counters. (__gcov_one_value_profiler_atomic): Ditto. (__gcov_indirect_call_profiler_atomic): Ditto. * gcc/gcov-io.h: Macros for atomic update. * gcc/common.opt: New option. * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic update profile counters. (gimple_gen_edge_profiler): Ditto. >>> >>> The patch looks resonable. Eventually we probably should provide rest of >>> the value counters >>> in thread safe manner. What happens on targets not having atomic >>> operations? >> >> From >> http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, >> it says: >> "If a particular operation cannot be implemented on the target >> processor, a warning is generated and a call an external function is >> generated. " >> >> So I think there will be a warning and eventually a link error of unsat. >> >> Thanks, >> >> -Rong >> >> >>> >>> Honza
Re: atomic update of profile counters (issue7000044)
Hi Honza, In the other thread of discussion (similar patch in google-4_7 branch), you said you were thinking if to let this patch into trunk in stage 3. Can you give some update? Thanks, -Rong On Fri, Dec 21, 2012 at 10:37 AM, Rong Xu wrote: > On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: >>> Hi, >>> >>> This patch adds support of atomic update of profiles counters. The goal is >>> to improve >>> the poor counter values for highly thread programs. >>> >>> The atomic update is under a new option -fprofile-gen-atomic= >>> N=0: default, no atomic update >>> N=1: atomic update edge counters. >>> N=2: atomic update some of value profile counters (currently indirect-call >>> and one value profile). >>> N=3: both edge counter and the above value profile counters. >>> Other value: fall back to the default. >>> >>> This patch is a simple porting of the version in google-4_7 branch. It uses >>> __atomic_fetch_add >>> based on Andrew Pinski's suggestion. Note I did not apply to all the value >>> profiles as >>> the indirect-call profile is the most relevant one here. >>> >>> Test with bootstrap. >>> >>> Comments and suggestions are welcomed. >>> >>> Thanks, >>> >>> -Rong >>> >>> >>> 2012-12-20 Rong Xu >>> >>> * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New >>> function. Atomic update profile counters. >>> (__gcov_one_value_profiler_atomic): Ditto. >>> (__gcov_indirect_call_profiler_atomic): Ditto. >>> * gcc/gcov-io.h: Macros for atomic update. >>> * gcc/common.opt: New option. >>> * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic >>> update profile counters. >>> (gimple_gen_edge_profiler): Ditto. >> >> The patch looks resonable. Eventually we probably should provide rest of >> the value counters >> in thread safe manner. What happens on targets not having atomic operations? > > From > http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, > it says: > "If a particular operation cannot be implemented on the target > processor, a warning is generated and a call an external function is > generated. " > > So I think there will be a warning and eventually a link error of unsat. > > Thanks, > > -Rong > > >> >> Honza
Re: atomic update of profile counters (issue7000044)
On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka wrote: >> Hi, >> >> This patch adds support of atomic update of profiles counters. The goal is >> to improve >> the poor counter values for highly thread programs. >> >> The atomic update is under a new option -fprofile-gen-atomic= >> N=0: default, no atomic update >> N=1: atomic update edge counters. >> N=2: atomic update some of value profile counters (currently indirect-call >> and one value profile). >> N=3: both edge counter and the above value profile counters. >> Other value: fall back to the default. >> >> This patch is a simple porting of the version in google-4_7 branch. It uses >> __atomic_fetch_add >> based on Andrew Pinski's suggestion. Note I did not apply to all the value >> profiles as >> the indirect-call profile is the most relevant one here. >> >> Test with bootstrap. >> >> Comments and suggestions are welcomed. >> >> Thanks, >> >> -Rong >> >> >> 2012-12-20 Rong Xu >> >> * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New >> function. Atomic update profile counters. >> (__gcov_one_value_profiler_atomic): Ditto. >> (__gcov_indirect_call_profiler_atomic): Ditto. >> * gcc/gcov-io.h: Macros for atomic update. >> * gcc/common.opt: New option. >> * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic >> update profile counters. >> (gimple_gen_edge_profiler): Ditto. > > The patch looks resonable. Eventually we probably should provide rest of the > value counters > in thread safe manner. What happens on targets not having atomic operations? >From >http://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins, it says: "If a particular operation cannot be implemented on the target processor, a warning is generated and a call an external function is generated. " So I think there will be a warning and eventually a link error of unsat. Thanks, -Rong > > Honza
Re: atomic update of profile counters (issue7000044)
> Hi, > > This patch adds support of atomic update of profiles counters. The goal is to > improve > the poor counter values for highly thread programs. > > The atomic update is under a new option -fprofile-gen-atomic= > N=0: default, no atomic update > N=1: atomic update edge counters. > N=2: atomic update some of value profile counters (currently indirect-call > and one value profile). > N=3: both edge counter and the above value profile counters. > Other value: fall back to the default. > > This patch is a simple porting of the version in google-4_7 branch. It uses > __atomic_fetch_add > based on Andrew Pinski's suggestion. Note I did not apply to all the value > profiles as > the indirect-call profile is the most relevant one here. > > Test with bootstrap. > > Comments and suggestions are welcomed. > > Thanks, > > -Rong > > > 2012-12-20 Rong Xu > > * libgcc/libgcov.c (__gcov_one_value_profiler_body_atomic): New > function. Atomic update profile counters. > (__gcov_one_value_profiler_atomic): Ditto. > (__gcov_indirect_call_profiler_atomic): Ditto. > * gcc/gcov-io.h: Macros for atomic update. > * gcc/common.opt: New option. > * gcc/tree-profile.c (gimple_init_edge_profiler): Atomic > update profile counters. > (gimple_gen_edge_profiler): Ditto. The patch looks resonable. Eventually we probably should provide rest of the value counters in thread safe manner. What happens on targets not having atomic operations? Honza
