Patchwork atomic update of profile counters (issue7000044)

login
register
mail settings
Submitter Rong Xu
Date Dec. 21, 2012, 6:45 a.m.
Message ID <20121221064539.0E1A7100704@rong.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/207763/
State New
Headers show

Comments

Rong Xu - Dec. 21, 2012, 6:45 a.m.
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
Jan Hubicka - Dec. 21, 2012, 9:25 a.m.
> 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
Rong Xu - Dec. 21, 2012, 6:37 p.m.
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
Rong Xu - Dec. 28, 2012, 7:32 p.m.
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
Xinliang David Li - Dec. 28, 2012, 7:35 p.m.
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
Rong Xu - Jan. 3, 2013, 1:15 a.m.
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
Andrew Pinski - Jan. 3, 2013, 1:25 a.m.
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
Rong Xu - Jan. 3, 2013, 1:29 a.m.
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
Andrew Pinski - Jan. 3, 2013, 1:31 a.m.
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
Richard Guenther - Jan. 3, 2013, 9:05 a.m.
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
Rong Xu - Jan. 4, 2013, 12:42 a.m.
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
Richard Henderson - Jan. 7, 2013, 8:36 p.m.
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~
Rong Xu - Jan. 7, 2013, 8:55 p.m.
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~

Patch

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);