| Message ID | 20121221064539.0E1A7100704@rong.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
> 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> > 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 <xur@google.com> > > * 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
On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >> 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 <xur@google.com> >> >> * 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
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 <xur@google.com> wrote: > On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>> 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 <xur@google.com> >>> >>> * 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
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 <xur@google.com> 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 <xur@google.com> wrote: >> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>> 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 <xur@google.com> >>>> >>>> * 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
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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>> 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 <xur@google.com> >>>>> >>>>> * 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
On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu <xur@google.com> 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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>>> 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 <xur@google.com> >>>>>> >>>>>> * 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
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 <pinskia@gmail.com> wrote: > On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu <xur@google.com> 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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>>>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>>>> 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 <xur@google.com> >>>>>>> >>>>>>> * 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
On Wed, Jan 2, 2013 at 5:29 PM, Rong Xu <xur@google.com> 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 <pinskia@gmail.com> wrote: >> On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu <xur@google.com> 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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>>>>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>>>>> 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 <xur@google.com> >>>>>>>> >>>>>>>> * 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
On Thu, Jan 3, 2013 at 2:25 AM, Andrew Pinski <pinskia@gmail.com> wrote: > On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu <xur@google.com> 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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>>>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>>>> 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 <xur@google.com> >>>>>>> >>>>>>> * 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
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 <richard.guenther@gmail.com> wrote: > On Thu, Jan 3, 2013 at 2:25 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Wed, Jan 2, 2013 at 5:15 PM, Rong Xu <xur@google.com> 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 <davidxl@google.com> 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 <xur@google.com> 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 <xur@google.com> wrote: >>>>>> On Fri, Dec 21, 2012 at 1:25 AM, Jan Hubicka <hubicka@ucw.cz> 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> >>>>>>>> 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 <xur@google.com> >>>>>>>> >>>>>>>> * 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
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~
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 <rth@redhat.com> 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~
Index: libgcc/libgcov.c =================================================================== --- libgcc/libgcov.c (revision 194652) +++ libgcc/libgcov.c (working copy) @@ -1113,12 +1113,35 @@ __gcov_one_value_profiler_body (gcov_type *counter counters[2]++; } +/* Atomic update version of __gcov_one_value_profile_body(). */ +static inline void +__gcov_one_value_profiler_body_atomic (gcov_type *counters, gcov_type value) +{ + if (value == counters[0]) + GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[1], 1, MEMMODEL_RELAXED); + else if (counters[1] == 0) + { + counters[1] = 1; + counters[0] = value; + } + else + GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[1], -1, MEMMODEL_RELAXED); + GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[2], 1, MEMMODEL_RELAXED); +} + #ifdef L_gcov_one_value_profiler void __gcov_one_value_profiler (gcov_type *counters, gcov_type value) { __gcov_one_value_profiler_body (counters, value); } + +void +__gcov_one_value_profiler_atomic (gcov_type *counters, gcov_type value) +{ + __gcov_one_value_profiler_body_atomic (counters, value); +} + #endif #ifdef L_gcov_indirect_call_profiler @@ -1153,6 +1176,17 @@ __gcov_indirect_call_profiler (gcov_type* counter, && *(void **) cur_func == *(void **) callee_func)) __gcov_one_value_profiler_body (counter, value); } + +/* Atomic update version of __gcov_indirect_call_profiler(). */ +void +__gcov_indirect_call_profiler_atomic (gcov_type* counter, gcov_type value, + void* cur_func, void* callee_func) +{ + if (cur_func == callee_func + || (VTABLE_USES_DESCRIPTORS && callee_func + && *(void **) cur_func == *(void **) callee_func)) + __gcov_one_value_profiler_body_atomic (counter, value); +} #endif Index: gcc/gcov-io.h =================================================================== --- gcc/gcov-io.h (revision 194652) +++ gcc/gcov-io.h (working copy) @@ -202,7 +202,15 @@ typedef unsigned gcov_type_unsigned __attribute__ #endif #endif +#if LONG_LONG_TYPE_SIZE > 32 +#define GCOV_TYPE_ATOMIC_FETCH_ADD_FN __atomic_fetch_add_8 +#define GCOV_TYPE_ATOMIC_FETCH_ADD BUILT_IN_ATOMIC_FETCH_ADD_8 +#else +#define GCOV_TYPE_ATOMIC_FETCH_ADD_FN __atomic_fetch_add_4 +#define GCOV_TYPE_ATOMIC_FETCH_ADD BUILT_IN_ATOMIC_FETCH_ADD_4 +#endif + #if defined (TARGET_POSIX_IO) #define GCOV_LOCKED 1 #else @@ -212,6 +220,18 @@ typedef unsigned gcov_type_unsigned __attribute__ #else /* !IN_LIBGCOV */ /* About the host */ +#if LONG_LONG_TYPE_SIZE > 32 +#define GCOV_TYPE_ATOMIC_FETCH_ADD_FN __atomic_fetch_add_8 +#define GCOV_TYPE_ATOMIC_FETCH_ADD BUILT_IN_ATOMIC_FETCH_ADD_8 +#else +#define GCOV_TYPE_ATOMIC_FETCH_ADD_FN __atomic_fetch_add_4 +#define GCOV_TYPE_ATOMIC_FETCH_ADD BUILT_IN_ATOMIC_FETCH_ADD_4 +#endif +#define PROFILE_GEN_EDGE_ATOMIC (flag_profile_gen_atomic == 1 || \ + flag_profile_gen_atomic == 3) +#define PROFILE_GEN_VALUE_ATOMIC (flag_profile_gen_atomic == 2 || \ + flag_profile_gen_atomic == 3) + typedef unsigned gcov_unsigned_t; typedef unsigned gcov_position_t; /* gcov_type is typedef'd elsewhere for the compiler */ Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 194652) +++ gcc/common.opt (working copy) @@ -1635,6 +1635,15 @@ fprofile-correction Common Report Var(flag_profile_correction) Enable correction of flow inconsistent profile data input +; fprofile-gen-atomic=0: disable atomically update. +; fprofile-gen-atomic=1: atomically update edge profile counters. +; fprofile-gen-atomic=2: atomically update value profile counters. +; fprofile-gen-atomic=3: atomically update edge and value profile counters. +; other values will be ignored (fall back to the default of 0). +fprofile-gen-atomic= +Common Joined UInteger Report Var(flag_profile_gen_atomic) Init(0) Optimization +fprofile-gen-atomic=[0..3] Atomically increments for 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 194652) +++ gcc/tree-profile.c (working copy) @@ -147,7 +147,12 @@ gimple_init_edge_profiler (void) = build_function_type_list (void_type_node, gcov_type_ptr, gcov_type_node, NULL_TREE); - tree_one_value_profiler_fn + if (PROFILE_GEN_VALUE_ATOMIC) + tree_one_value_profiler_fn + = build_fn_decl ("__gcov_one_value_profiler_atomic", + one_value_profiler_fn_type); + else + tree_one_value_profiler_fn = build_fn_decl ("__gcov_one_value_profiler", one_value_profiler_fn_type); TREE_NOTHROW (tree_one_value_profiler_fn) = 1; @@ -163,9 +168,14 @@ gimple_init_edge_profiler (void) gcov_type_ptr, gcov_type_node, ptr_void, ptr_void, NULL_TREE); - tree_indirect_call_profiler_fn - = build_fn_decl ("__gcov_indirect_call_profiler", - ic_profiler_fn_type); + if (PROFILE_GEN_VALUE_ATOMIC) + tree_indirect_call_profiler_fn + = build_fn_decl ("__gcov_indirect_call_profiler_atomic", + ic_profiler_fn_type); + else + tree_indirect_call_profiler_fn + = build_fn_decl ("__gcov_indirect_call_profiler", + ic_profiler_fn_type); TREE_NOTHROW (tree_indirect_call_profiler_fn) = 1; DECL_ATTRIBUTES (tree_indirect_call_profiler_fn) = tree_cons (get_identifier ("leaf"), NULL, @@ -211,8 +221,21 @@ gimple_gen_edge_profiler (int edgeno, edge e) tree ref, one, gcov_type_tmp_var; gimple stmt1, stmt2, stmt3; + one = build_int_cst (gcov_type_node, 1); + if (PROFILE_GEN_EDGE_ATOMIC) + { + ref = tree_coverage_counter_addr (GCOV_COUNTER_ARCS, edgeno); + /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */ + stmt1 = gimple_build_call (builtin_decl_explicit ( + GCOV_TYPE_ATOMIC_FETCH_ADD), + 3, ref, one, + build_int_cst (integer_type_node, + MEMMODEL_RELAXED)); + gsi_insert_on_edge (e, stmt1); + return; + } + ref = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno); - one = build_int_cst (gcov_type_node, 1); gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node, NULL, "PROF_edge_counter"); stmt1 = gimple_build_assign (gcov_type_tmp_var, ref);
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> 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 <xur@google.com> * 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. -- This patch is available for review at http://codereview.appspot.com/7000044