atomic update of profile counters

[xur, 2012-12-21 06:45:41]

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.

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

--
This patch is available for review at http://codereview.appspot.com/7000044

[hubicka_ucw.cz, 2012-12-21 09:25:32]

> 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

[xur, 2012-12-21 18:37:46]

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...,
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

[xur, 2012-12-28 19:32:49]

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...,
> 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

[davidxl, 2012-12-28 19:35:13]

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...,
>> 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

[xur, 2013-01-03 01:15:53]

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...,
>>> 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

[pinskia_gmail.com, 2013-01-03 01:25:24]

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...,
>>>> 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

[xur, 2013-01-03 01:29:44]

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...,
>>>>> 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

[pinskia_gmail.com, 2013-01-03 01:31:42]

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...,
>>>>>> 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_gmail.com, 2013-01-03 09:05:22]

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...,
>>>>> 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

[xur, 2013-01-04 00:42:18]

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...,
>>>>>> 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

[rth_redhat.com, 2013-01-07 20:36:10]

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~

[xur, 2013-01-07 20:55:56]

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~

[xur, 2013-11-20 01:02:41]

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 <xur@google.com> 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 <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~

[pinskia_gmail.com, 2013-11-20 01:11:50]

On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu <xur@google.com> 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 <xur@google.com> 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 <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~

[xur, 2013-11-20 18:44:07]

On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu <xur@google.com> 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 =>  (0x00007fff56bff000)
libatomic.so.1 => not found
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00002b0720261000)
/lib64/ld-linux-x86-64.so.2 (0x00002b072003c000)

>> ./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=<gcc_install_patch>/lib64 ./a.out
works fine.

I think that's the same reason we set the library path in
testsuite/lib/atomic-dg.exp, because <gcc_install_patch>/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 <xur@google.com> 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 <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~

[pinskia_gmail.com, 2013-11-20 18:48:29]

On Wed, Nov 20, 2013 at 10:44 AM, Rong Xu <xur@google.com> wrote:
> On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu <xur@google.com> 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 =>  (0x00007fff56bff000)
> libatomic.so.1 => not found
> libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00002b0720261000)
> /lib64/ld-linux-x86-64.so.2 (0x00002b072003c000)
>
>>> ./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=<gcc_install_patch>/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 <gcc_install_patch>/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 <xur@google.com> 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 <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~

[pinskia_gmail.com, 2013-11-20 18:59:22]

On Wed, Nov 20, 2013 at 10:48 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Nov 20, 2013 at 10:44 AM, Rong Xu <xur@google.com> wrote:
>> On Tue, Nov 19, 2013 at 5:11 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Tue, Nov 19, 2013 at 5:02 PM, Rong Xu <xur@google.com> 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 =>  (0x00007fff56bff000)
>> libatomic.so.1 => not found
>> libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00002b0720261000)
>> /lib64/ld-linux-x86-64.so.2 (0x00002b072003c000)
>>
>>>> ./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=<gcc_install_patch>/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 <gcc_install_patch>/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 <xur@google.com> 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 <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~

[joseph_codesourcery.com, 2013-11-20 21:04:38]

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
joseph@codesourcery.com

[xur, 2013-11-20 22:07:22]

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
<joseph@codesourcery.com> 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
> joseph@codesourcery.com

[pinskia_gmail.com, 2013-11-20 22:19:25]

On Wed, Nov 20, 2013 at 2:07 PM, Rong Xu <xur@google.com> 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
> <joseph@codesourcery.com> 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
>> joseph@codesourcery.com

[xur, 2013-11-20 22:28:26]

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 <pinskia@gmail.com> wrote:
> On Wed, Nov 20, 2013 at 2:07 PM, Rong Xu <xur@google.com> 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
>> <joseph@codesourcery.com> 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
>>> joseph@codesourcery.com

[hubicka_ucw.cz, 2014-05-26 06:01:11]

> 2013-11-19  Rong Xu  <xur@google.com>
> 
> 	* 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.