Re: atomic update of profile counters (issue7000044)

[Rong Xu <xur@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

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~

[-- Attachment #2: libgcov_atomic_new.txt --]
[-- Type: text/plain, Size: 17702 bytes --]

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.
+
 fprofile-generate
 Common
 Enable common options for generating profile info for profile feedback directed optimizations
Index: gcc/tree-profile.c
===================================================================
--- gcc/tree-profile.c	(revision 205053)
+++ gcc/tree-profile.c	(working copy)
@@ -155,6 +155,9 @@ gimple_init_edge_profiler (void)
   tree ic_profiler_fn_type;
   tree average_profiler_fn_type;
   tree time_profiler_fn_type;
+  const char *fn_name;
+  bool profile_gen_value_atomic = (flag_profile_generate_atomic == 2 ||
+                                   flag_profile_generate_atomic == 3);
 
   if (!gcov_type_node)
     {
@@ -167,9 +170,10 @@ gimple_init_edge_profiler (void)
 					  gcov_type_ptr, gcov_type_node,
 					  integer_type_node,
 					  unsigned_type_node, NULL_TREE);
+      fn_name = profile_gen_value_atomic ? "__gcov_interval_profiler_atomic" : 
+                 "__gcov_interval_profiler";
       tree_interval_profiler_fn
-	      = build_fn_decl ("__gcov_interval_profiler",
-				     interval_profiler_fn_type);
+	      = build_fn_decl (fn_name, interval_profiler_fn_type);
       TREE_NOTHROW (tree_interval_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_interval_profiler_fn)
 	= tree_cons (get_identifier ("leaf"), NULL,
@@ -180,21 +184,23 @@ gimple_init_edge_profiler (void)
 	      = build_function_type_list (void_type_node,
 					  gcov_type_ptr, gcov_type_node,
 					  NULL_TREE);
-      tree_pow2_profiler_fn = build_fn_decl ("__gcov_pow2_profiler",
-						   pow2_profiler_fn_type);
+      fn_name = profile_gen_value_atomic ? "__gcov_pow2_profiler_atomic" : 
+                 "__gcov_pow2_profiler";
+      tree_pow2_profiler_fn = build_fn_decl (fn_name, pow2_profiler_fn_type);
       TREE_NOTHROW (tree_pow2_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_pow2_profiler_fn)
 	= tree_cons (get_identifier ("leaf"), NULL,
 		     DECL_ATTRIBUTES (tree_pow2_profiler_fn));
 
       /* void (*) (gcov_type *, gcov_type)  */
+      fn_name = profile_gen_value_atomic ? "__gcov_one_value_profiler_atomic" :
+                 "__gcov_one_value_profiler";
       one_value_profiler_fn_type
 	      = build_function_type_list (void_type_node,
 					  gcov_type_ptr, gcov_type_node,
 					  NULL_TREE);
       tree_one_value_profiler_fn
-	      = build_fn_decl ("__gcov_one_value_profiler",
-				     one_value_profiler_fn_type);
+	      = build_fn_decl (fn_name, one_value_profiler_fn_type);
       TREE_NOTHROW (tree_one_value_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_one_value_profiler_fn)
 	= tree_cons (get_identifier ("leaf"), NULL,
@@ -211,9 +217,10 @@ gimple_init_edge_profiler (void)
 					      gcov_type_ptr, gcov_type_node,
 					      ptr_void, ptr_void,
 					      NULL_TREE);
+          fn_name = profile_gen_value_atomic ? "__gcov_indirect_call_profiler_atomic" : 
+                     "__gcov_indirect_call_profiler";
 	  tree_indirect_call_profiler_fn
-		  = build_fn_decl ("__gcov_indirect_call_profiler",
-					 ic_profiler_fn_type);
+		  = build_fn_decl (fn_name, ic_profiler_fn_type);
         }
       else
         {
@@ -223,9 +230,10 @@ gimple_init_edge_profiler (void)
 					      gcov_type_node,
 					      ptr_void,
 					      NULL_TREE);
+          fn_name = profile_gen_value_atomic ? "__gcov_indirect_call_profiler_v2_atomic" : 
+                     "__gcov_indirect_call_profiler_v2";
 	  tree_indirect_call_profiler_fn
-		  = build_fn_decl ("__gcov_indirect_call_profiler_v2",
-					 ic_profiler_fn_type);
+		  = build_fn_decl (fn_name, ic_profiler_fn_type);
         }
       TREE_NOTHROW (tree_indirect_call_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_indirect_call_profiler_fn)
@@ -236,9 +244,10 @@ gimple_init_edge_profiler (void)
       time_profiler_fn_type
 	       = build_function_type_list (void_type_node,
 					  gcov_type_ptr, NULL_TREE);
+      fn_name = profile_gen_value_atomic ? "__gcov_time_profiler_atomic" : 
+                 "__gcov_time_profiler";
       tree_time_profiler_fn
-	      = build_fn_decl ("__gcov_time_profiler",
-				     time_profiler_fn_type);
+	      = build_fn_decl (fn_name, time_profiler_fn_type);
       TREE_NOTHROW (tree_time_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_time_profiler_fn)
 	= tree_cons (get_identifier ("leaf"), NULL,
@@ -248,9 +257,10 @@ gimple_init_edge_profiler (void)
       average_profiler_fn_type
 	      = build_function_type_list (void_type_node,
 					  gcov_type_ptr, gcov_type_node, NULL_TREE);
+      fn_name = profile_gen_value_atomic ? "__gcov_average_profiler_atomic" : 
+                 "__gcov_average_profiler";
       tree_average_profiler_fn
-	      = build_fn_decl ("__gcov_average_profiler",
-				     average_profiler_fn_type);
+	      = build_fn_decl (fn_name, average_profiler_fn_type);
       TREE_NOTHROW (tree_average_profiler_fn) = 1;
       DECL_ATTRIBUTES (tree_average_profiler_fn)
 	= tree_cons (get_identifier ("leaf"), NULL,
@@ -284,9 +294,24 @@ gimple_gen_edge_profiler (int edgeno, edge e)
 {
   tree ref, one, gcov_type_tmp_var;
   gimple stmt1, stmt2, stmt3;
+  bool profile_gen_edge_atomic = (flag_profile_generate_atomic == 1 ||
+                                  flag_profile_generate_atomic == 3);
 
+  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);
Index: gcc/gcov-io.h
===================================================================
--- gcc/gcov-io.h	(revision 205053)
+++ gcc/gcov-io.h	(working copy)
@@ -211,6 +211,15 @@ typedef unsigned gcov_type_unsigned __attribute__
 #else /* !IN_LIBGCOV */
 /* About the host */
 
+/* Macros for atomical counter update.  */
+#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
+
 typedef unsigned gcov_unsigned_t;
 typedef unsigned gcov_position_t;
 /* gcov_type is typedef'd elsewhere for the compiler */
Index: libgcc/libgcov-profiler.c
===================================================================
--- libgcc/libgcov-profiler.c	(revision 205053)
+++ libgcc/libgcov-profiler.c	(working copy)
@@ -33,6 +33,15 @@ see the files COPYING3 and COPYING.RUNTIME respect
 #define IN_LIBGCOV 1
 #include "gcov-io.h"
 
+/* Macros for atomical counter update.  */
+#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
+
 #ifdef L_gcov_interval_profiler
 /* If VALUE is in interval <START, START + STEPS - 1>, then increases the
    corresponding counter in COUNTERS.  If the VALUE is above or below
@@ -53,6 +62,21 @@ __gcov_interval_profiler (gcov_type *counters, gco
 }
 #endif
 
+#ifdef L_gcov_interval_profiler_atomic
+void
+__gcov_interval_profiler_atomic (gcov_type *counters, gcov_type value,
+                                 int start, unsigned steps)
+{
+  gcov_type delta = value - start;
+  if (delta < 0)
+    GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[steps + 1], 1, MEMMODEL_RELAXED);
+  else if (delta >= steps)
+    GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[steps], 1, MEMMODEL_RELAXED);
+  else
+    GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[delta], 1, MEMMODEL_RELAXED);
+}
+#endif
+
 #ifdef L_gcov_pow2_profiler
 /* If VALUE is a power of two, COUNTERS[1] is incremented.  Otherwise
    COUNTERS[0] is incremented.  */
@@ -67,6 +91,17 @@ __gcov_pow2_profiler (gcov_type *counters, gcov_ty
 }
 #endif
 
+#ifdef L_gcov_pow2_profiler_atomic
+void
+__gcov_pow2_profiler_atomic (gcov_type *counters, gcov_type value)
+{
+  if (value & (value - 1))
+    GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[0], 1, MEMMODEL_RELAXED);
+  else
+    GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[1], 1, MEMMODEL_RELAXED);
+}
+#endif
+
 /* Tries to determine the most common value among its inputs.  Checks if the
    value stored in COUNTERS[0] matches VALUE.  If this is the case, COUNTERS[1]
    is incremented.  If this is not the case and COUNTERS[1] is not zero,
@@ -92,6 +127,22 @@ __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)
@@ -100,9 +151,13 @@ __gcov_one_value_profiler (gcov_type *counters, gc
 }
 #endif
 
-#ifdef L_gcov_indirect_call_profiler
-/* This function exist only for workaround of binutils bug 14342.
-   Once this compatibility hack is obsolette, it can be removed.  */
+#ifdef L_gcov_one_value_profiler_atomic
+void
+__gcov_one_value_profiler_atomic (gcov_type *counters, gcov_type value)
+{
+  __gcov_one_value_profiler_body_atomic (counters, value);
+}
+#endif
 
 /* By default, the C++ compiler will use function addresses in the
    vtable entries.  Setting TARGET_VTABLE_USES_DESCRIPTORS to nonzero
@@ -121,6 +176,10 @@ __gcov_one_value_profiler (gcov_type *counters, gc
 #define VTABLE_USES_DESCRIPTORS 0
 #endif
 
+#ifdef L_gcov_indirect_call_profiler
+/* This function exist only for workaround of binutils bug 14342.
+   Once this compatibility hack is obsolette, it can be removed.  */
+
 /* Tries to determine the most common value among its inputs. */
 void
 __gcov_indirect_call_profiler (gcov_type* counter, gcov_type value,
@@ -134,8 +193,21 @@ __gcov_indirect_call_profiler (gcov_type* counter,
           && *(void **) cur_func == *(void **) callee_func))
     __gcov_one_value_profiler_body (counter, value);
 }
+#endif
 
+#ifdef L_gcov_indirect_call_profiler_atomic
+/* 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
+
 #ifdef L_gcov_indirect_call_profiler_v2
 
 /* These two variables are used to actually track caller and callee.  Keep
@@ -152,23 +224,6 @@ __thread
 #endif
 gcov_type * __gcov_indirect_call_counters;
 
-/* By default, the C++ compiler will use function addresses in the
-   vtable entries.  Setting TARGET_VTABLE_USES_DESCRIPTORS to nonzero
-   tells the compiler to use function descriptors instead.  The value
-   of this macro says how many words wide the descriptor is (normally 2),
-   but it may be dependent on target flags.  Since we do not have access
-   to the target flags here we just check to see if it is set and use
-   that to set VTABLE_USES_DESCRIPTORS to 0 or 1.
-
-   It is assumed that the address of a function descriptor may be treated
-   as a pointer to a function.  */
-
-#ifdef TARGET_VTABLE_USES_DESCRIPTORS
-#define VTABLE_USES_DESCRIPTORS 1
-#else
-#define VTABLE_USES_DESCRIPTORS 0
-#endif
-
 /* Tries to determine the most common value among its inputs. */
 void
 __gcov_indirect_call_profiler_v2 (gcov_type value, void* cur_func)
@@ -183,10 +238,30 @@ __gcov_indirect_call_profiler_v2 (gcov_type value,
 }
 #endif
 
+#ifdef L_gcov_indirect_call_profiler_v2_atomic
+
+#if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
+extern __thread void * __gcov_indirect_call_callee;
+extern __thread gcov_type * __gcov_indirect_call_counters;
+#else
+extern void * __gcov_indirect_call_callee;
+extern gcov_type * __gcov_indirect_call_counters;
+#endif
+
+void
+__gcov_indirect_call_profiler_v2_atomic (gcov_type value, void* cur_func)
+{
+  if (cur_func == __gcov_indirect_call_callee
+      || (VTABLE_USES_DESCRIPTORS && __gcov_indirect_call_callee
+          && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
+    __gcov_one_value_profiler_body_atomic (__gcov_indirect_call_counters, value);
+}
+#endif
+
 #ifdef L_gcov_time_profiler
 
 /* Counter for first visit of each function.  */
-static gcov_type function_counter;
+gcov_type function_counter;
 
 /* Sets corresponding COUNTERS if there is no value.  */
 
@@ -198,6 +273,16 @@ __gcov_time_profiler (gcov_type* counters)
 }
 #endif
 
+#ifdef L_gcov_time_profiler_atomic
+extern gcov_type function_counter;
+void
+__gcov_time_profiler_atomic (gcov_type* counters)
+{
+  if (!counters[0])
+    counters[0] = GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&function_counter, 1, MEMMODEL_RELAXED);
+}
+#endif
+
 #ifdef L_gcov_average_profiler
 /* Increase corresponding COUNTER by VALUE.  FIXME: Perhaps we want
    to saturate up.  */
@@ -205,11 +290,20 @@ __gcov_time_profiler (gcov_type* counters)
 void
 __gcov_average_profiler (gcov_type *counters, gcov_type value)
 {
-  counters[0] += value;
-  counters[1] ++;
+  GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[0], value, MEMMODEL_RELAXED);
+  GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[1], 1, MEMMODEL_RELAXED);
 }
 #endif
 
+#ifdef L_gcov_average_profiler_atomic
+void
+__gcov_average_profiler_atomic (gcov_type *counters, gcov_type value)
+{
+  GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[0], value, MEMMODEL_RELAXED);
+  GCOV_TYPE_ATOMIC_FETCH_ADD_FN (&counters[1], 1, MEMMODEL_RELAXED);
+}
+#endif
+
 #ifdef L_gcov_ior_profiler
 /* Bitwise-OR VALUE into COUNTER.  */
 
Index: libgcc/Makefile.in
===================================================================
--- libgcc/Makefile.in	(revision 205053)
+++ libgcc/Makefile.in	(working copy)
@@ -863,15 +863,20 @@ LIBGCOV_DRIVER = _gcov
 
 libgcov-merge-objects = $(patsubst %,%$(objext),$(LIBGCOV_MERGE))
 libgcov-profiler-objects = $(patsubst %,%$(objext),$(LIBGCOV_PROFILER))
+libgcov-profiler-atomic-objects = $(patsubst %,%_atomic$(objext),$(LIBGCOV_PROFILER))
 libgcov-interface-objects = $(patsubst %,%$(objext),$(LIBGCOV_INTERFACE))
 libgcov-driver-objects = $(patsubst %,%$(objext),$(LIBGCOV_DRIVER))
 libgcov-objects = $(libgcov-merge-objects) $(libgcov-profiler-objects) \
-                 $(libgcov-interface-objects) $(libgcov-driver-objects)
+                 $(libgcov-interface-objects) $(libgcov-driver-objects) \
+                 $(libgcov-profiler-atomic-objects)
 
+
 $(libgcov-merge-objects): %$(objext): $(srcdir)/libgcov-merge.c
 	$(gcc_compile) -DL$* -c $(srcdir)/libgcov-merge.c
 $(libgcov-profiler-objects): %$(objext): $(srcdir)/libgcov-profiler.c
 	$(gcc_compile) -DL$* -c $(srcdir)/libgcov-profiler.c
+$(libgcov-profiler-atomic-objects): %$(objext): $(srcdir)/libgcov-profiler.c
+	$(gcc_compile) -DL$* -c $(srcdir)/libgcov-profiler.c
 $(libgcov-interface-objects): %$(objext): $(srcdir)/libgcov-interface.c
 	$(gcc_compile) -DL$* -c $(srcdir)/libgcov-interface.c
 $(libgcov-driver-objects): %$(objext): $(srcdir)/libgcov-driver.c \