On 08/01/2016 02:22 PM, Nathan Sidwell wrote:
> As I just wrote, this patch needs work. the general points are:
Thank for the comments.
> 1) exposing integers 0-3 to the user as switch values. Don't do that, give
> them names. In this case a comma separated list of orthogonal names seems
> appropriate. But see below.
> 2) Poor documentation. How might the user might choose an appropriate
> setting? (what happens if compilations need to use different settings). What
> are 'edge' and 'value' counters. Why might one want different settings for
> them?
Sure, fully agree that it currently doesn't make sense to distinguish between
individual types of profiles (edge, value).
>
> I think this is jumping too deep into a solution with insufficient evidence.
> Particularly, why two edges and values can be set differently. It doesn't
> lend itself to extending to TLS, if that proves to be a good solution
> (trading memory for time). Something along the lines of
> '-fprofile-update={single,atomic,threaded},[edge,value]' might be better.
> I.e. set the scheme as part of the option value, followed by a list of the
> things it applies to. (and as I hope I've implied, it'd be good not to have
> that separate list until proven otherwise).
Yes.
>
>
> On 07/28/16 08:32, marxin wrote:
>> libgcc/ChangeLog:
>>
>> 2016-07-28 Martin Liska <[email protected]>
>
> Shouldn't the original authors be named here too? (applies to the other
> patches too).
Adding a cherry-pick entry to the original commit of the functionality.
>
>
>> --- a/gcc/gcov-io.h
>> +++ b/gcc/gcov-io.h
>> @@ -169,6 +169,19 @@ see the files COPYING3 and COPYING.RUNTIME
>> respectively. If not, see
>
>> +
>> +#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 IN_LIBGCOV
>> +
>> +#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
>
>
> BTW, these two blocks look stunningly similar.
Fixed.
I also added a small hunk that describes problematic of app having not-joined
(or detached) threads,
can you please take a look at documentation change, maybe it would need some
transformation?
Martin
>
> nathan
>From 7e19e28f3d6e227bb67fb770575831d637abe3aa Mon Sep 17 00:00:00 2001
From: marxin <[email protected]>
Date: Thu, 28 Jul 2016 14:32:47 +0200
Subject: [PATCH 1/4] Cherry-pick fprofile-generate-atomic from google/gcc-4_9
branch
libgcc/ChangeLog:
2016-07-28 Martin Liska <[email protected]>
Cherry picked (and modified) from google-4_7 branch
2012-12-26 Rong Xu <[email protected]>
* Makefile.in: Add functions to LIBGCOV_PROFILER.
* libgcov-profiler.c (__gcov_one_value_profiler_body_atomic):
New function.
(__gcov_one_value_profiler_atomic): Likewise.
(__gcov_indirect_call_profiler_v2): Fix GNU coding style.
(__gcov_indirect_call_profiler_v2_atomic): New function.
* libgcov.h: Declare __gcov_indirect_call_profiler_v2_atomic and
__gcov_one_value_profiler_body_atomic.
gcc/ChangeLog:
2016-07-28 Martin Liska <[email protected]>
Cherry picked (and modified) from google-4_7 branch
2012-12-26 Rong Xu <[email protected]>
* common.opt (fprofile-update): Add new flag.
* gcov-io.h: Declare GCOV_TYPE_ATOMIC_FETCH_ADD and
GCOV_TYPE_ATOMIC_FETCH_ADD_FN.
* tree-profile.c (gimple_init_edge_profiler): Generate
also atomic profiler update.
(gimple_gen_edge_profiler): Likewise.
* doc/invoke.texi: Document -fprofile-update.
gcc/testsuite/ChangeLog:
2016-07-28 Martin Liska <[email protected]>
* g++.dg/gcov/gcov-threads-1.C: New test.
---
gcc/common.opt | 13 +++++++
gcc/coretypes.h | 6 +++
gcc/doc/invoke.texi | 12 ++++++
gcc/gcov-io.h | 8 ++++
gcc/testsuite/g++.dg/gcov/gcov-threads-1.C | 46 ++++++++++++++++++++++
gcc/tree-profile.c | 61 ++++++++++++++++++++----------
libgcc/Makefile.in | 4 +-
libgcc/libgcov-profiler.c | 42 +++++++++++++++++++-
libgcc/libgcov.h | 2 +
9 files changed, 173 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/gcov/gcov-threads-1.C
diff --git a/gcc/common.opt b/gcc/common.opt
index 8a292ed..44adae8 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1916,6 +1916,19 @@ fprofile-correction
Common Report Var(flag_profile_correction)
Enable correction of flow inconsistent profile data input.
+fprofile-update=
+Common Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE)
+-fprofile-update=[single|atomic] Set the profile update method.
+
+Enum
+Name(profile_update) Type(enum profile_update) UnknownError(unknown profile update method %qs)
+
+EnumValue
+Enum(profile_update) String(single) Value(PROFILE_UPDATE_SINGLE)
+
+EnumValue
+Enum(profile_update) String(atomic) Value(PROFILE_UPDATE_ATOMIC)
+
fprofile-generate
Common
Enable common options for generating profile info for profile feedback directed optimizations.
diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index b3a91a6..fe1e984 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -174,6 +174,12 @@ enum offload_abi {
OFFLOAD_ABI_ILP32
};
+/* Types of profile update methods. */
+enum profile_update {
+ PROFILE_UPDATE_SINGLE,
+ PROFILE_UPDATE_ATOMIC
+};
+
/* Types of unwind/exception handling info that can be generated. */
enum unwind_info_type
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 22001f9..1cfaae7 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9933,6 +9933,18 @@ the profile feedback data files. See @option{-fprofile-dir}.
To optimize the program based on the collected profile information, use
@option{-fprofile-use}. @xref{Optimize Options}, for more information.
+@item -fprofile-update=@var{method}
+@opindex fprofile-update
+
+Alter the update method for an application instrumented for profile
+feedback based optimization. The @var{method} argument should be one of
+@samp{single} or @samp{atomic}. The first one is useful for single-threaded
+applications, while the second one prevents profile corruption by emitting
+thread-safe code.
+
+@strong{Warning:} When an application does not properly join all threads
+(or creates an detached thread), a profile file can be still corrupted.
+
@item -fsanitize=address
@opindex fsanitize=address
Enable AddressSanitizer, a fast memory error detector.
diff --git a/gcc/gcov-io.h b/gcc/gcov-io.h
index bbf013a..afd00ac 100644
--- a/gcc/gcov-io.h
+++ b/gcc/gcov-io.h
@@ -164,6 +164,14 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
#ifndef GCC_GCOV_IO_H
#define GCC_GCOV_IO_H
+#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
+
#ifndef IN_LIBGCOV
/* About the host */
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-threads-1.C b/gcc/testsuite/g++.dg/gcov/gcov-threads-1.C
new file mode 100644
index 0000000..a4a6f0a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/gcov/gcov-threads-1.C
@@ -0,0 +1,46 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage -pthread -fprofile-update=atomic" } */
+/* { dg-do run { target native } } */
+
+#include <stdint.h>
+#include <pthread.h>
+#include <assert.h>
+
+#define NR 5
+
+pthread_mutex_t cndMs[NR];
+static void *ContentionNoDeadlock_thread(void *start)
+{
+ for (uint32_t k = 0; k < 100000; ++k) /* count(500005) */
+ {
+ int starti = *(int*)start; /* count(500000) */
+ for (uint32_t i = starti; i < NR; ++i)
+ pthread_mutex_lock (&cndMs[i]);
+ for (int32_t i = NR - 1; i >= starti; --i)
+ pthread_mutex_unlock (&cndMs[i]);
+ }
+}
+int main(int argc, char **argv) {
+ for (unsigned i = 0; i < NR; i++)
+ cndMs[i] = PTHREAD_MUTEX_INITIALIZER;
+
+ pthread_t t[NR];
+ int ids[NR];
+
+ for (int i = 0; i < NR; i++)
+ {
+ ids[i] = i;
+ int r = pthread_create (&t[i], NULL, ContentionNoDeadlock_thread, &ids[i]);
+ assert (r == 0); /* count(5) */
+ }
+
+ int ret;
+ for (int i = 0; i < NR; i++)
+ {
+ int r = pthread_join (t[i], (void**)&ret);
+ assert (r == 0); /* count(5) */
+ }
+
+ return 0; /* count(1) */
+}
+
+/* { dg-final { run-gcov gcov-threads-1.C } } */
diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
index 39fe15f..a2c86ac 100644
--- a/gcc/tree-profile.c
+++ b/gcc/tree-profile.c
@@ -164,7 +164,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 (flag_profile_update == PROFILE_UPDATE_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;
@@ -180,11 +185,14 @@ gimple_init_edge_profiler (void)
gcov_type_node,
ptr_void,
NULL_TREE);
+ const char *profiler_fn_name = "__gcov_indirect_call_profiler_v2";
+ if (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE))
+ profiler_fn_name = "__gcov_indirect_call_topn_profiler";
+ if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
+ profiler_fn_name = "__gcov_indirect_call_profiler_v2_atomic";
+
tree_indirect_call_profiler_fn
- = build_fn_decl ( (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
- "__gcov_indirect_call_topn_profiler":
- "__gcov_indirect_call_profiler_v2"),
- ic_profiler_fn_type);
+ = build_fn_decl (profiler_fn_name, ic_profiler_fn_type);
TREE_NOTHROW (tree_indirect_call_profiler_fn) = 1;
DECL_ATTRIBUTES (tree_indirect_call_profiler_fn)
@@ -241,22 +249,37 @@ gimple_init_edge_profiler (void)
void
gimple_gen_edge_profiler (int edgeno, edge e)
{
- tree ref, one, gcov_type_tmp_var;
- gassign *stmt1, *stmt2, *stmt3;
+ tree one;
- 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);
- gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
- NULL, "PROF_edge_counter");
- stmt2 = gimple_build_assign (gcov_type_tmp_var, PLUS_EXPR,
- gimple_assign_lhs (stmt1), one);
- stmt3 = gimple_build_assign (unshare_expr (ref), gimple_assign_lhs (stmt2));
- gsi_insert_on_edge (e, stmt1);
- gsi_insert_on_edge (e, stmt2);
- gsi_insert_on_edge (e, stmt3);
+
+ if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
+ {
+ /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */
+ tree addr = tree_coverage_counter_addr (GCOV_COUNTER_ARCS, edgeno);
+ gcall *stmt
+ = gimple_build_call (builtin_decl_explicit (GCOV_TYPE_ATOMIC_FETCH_ADD),
+ 3, addr, one,
+ build_int_cst (integer_type_node,
+ MEMMODEL_RELAXED));
+ gsi_insert_on_edge (e, stmt);
+ }
+ else
+ {
+ tree ref = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno);
+ tree gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
+ NULL, "PROF_edge_counter");
+ gassign *stmt1 = gimple_build_assign (gcov_type_tmp_var, ref);
+ gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
+ NULL, "PROF_edge_counter");
+ gassign *stmt2 = gimple_build_assign (gcov_type_tmp_var, PLUS_EXPR,
+ gimple_assign_lhs (stmt1), one);
+ gassign *stmt3 = gimple_build_assign (unshare_expr (ref),
+ gimple_assign_lhs (stmt2));
+ gsi_insert_on_edge (e, stmt1);
+ gsi_insert_on_edge (e, stmt2);
+ gsi_insert_on_edge (e, stmt3);
+ }
}
/* Emits code to get VALUE to instrument at GSI, and returns the
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index f09b39b..8b0fdd9 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -860,8 +860,10 @@ LIBGCOV_MERGE = _gcov_merge_add _gcov_merge_single _gcov_merge_delta \
_gcov_merge_ior _gcov_merge_time_profile _gcov_merge_icall_topn
LIBGCOV_PROFILER = _gcov_interval_profiler _gcov_pow2_profiler \
_gcov_one_value_profiler _gcov_indirect_call_profiler \
+ _gcov_one_value_profiler_atomic \
_gcov_average_profiler _gcov_ior_profiler \
- _gcov_indirect_call_profiler_v2 _gcov_time_profiler \
+ _gcov_indirect_call_profiler_v2 \
+ _gcov_time_profiler \
_gcov_indirect_call_topn_profiler
LIBGCOV_INTERFACE = _gcov_dump _gcov_flush _gcov_fork \
_gcov_execl _gcov_execlp \
diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
index e947188..1b307ac 100644
--- a/libgcc/libgcov-profiler.c
+++ b/libgcc/libgcov-profiler.c
@@ -93,6 +93,31 @@ __gcov_one_value_profiler (gcov_type *counters, gcov_type value)
}
#endif
+/* 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_atomic
+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_topn_profiler
/* Tries to keep track the most frequent N values in the counters where
N is specified by parameter TOPN_VAL. To track top N values, 2*N counter
@@ -229,6 +254,7 @@ __gcov_indirect_call_topn_profiler (gcov_type value, void* cur_func)
&& *(void **) cur_func == *(void **) callee_func))
__gcov_topn_value_profiler_body (__gcov_indirect_call_topn_counters, value);
}
+
#endif
#ifdef L_gcov_indirect_call_profiler
@@ -291,9 +317,23 @@ __gcov_indirect_call_profiler_v2 (gcov_type value, void* cur_func)
the descriptors to see if they point to the same function. */
if (cur_func == __gcov_indirect_call_callee
|| (__LIBGCC_VTABLE_USES_DESCRIPTORS__ && __gcov_indirect_call_callee
- && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
+ && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
__gcov_one_value_profiler_body (__gcov_indirect_call_counters, value);
}
+
+/* Atomic update version of __gcov_indirect_call_profiler_v2(). */
+void
+__gcov_indirect_call_profiler_v2_atomic (gcov_type value, void* cur_func)
+{
+ /* If the C++ virtual tables contain function descriptors then one
+ function may have multiple descriptors and we need to dereference
+ the descriptors to see if they point to the same function. */
+ if (cur_func == __gcov_indirect_call_callee
+ || (__LIBGCC_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
diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
index ae77998..0bd905b 100644
--- a/libgcc/libgcov.h
+++ b/libgcc/libgcov.h
@@ -270,9 +270,11 @@ extern void __gcov_merge_icall_topn (gcov_type *, unsigned) ATTRIBUTE_HIDDEN;
extern void __gcov_interval_profiler (gcov_type *, gcov_type, int, unsigned);
extern void __gcov_pow2_profiler (gcov_type *, gcov_type);
extern void __gcov_one_value_profiler (gcov_type *, gcov_type);
+extern void __gcov_one_value_profiler_atomic (gcov_type *, gcov_type);
extern void __gcov_indirect_call_profiler (gcov_type*, gcov_type,
void*, void*);
extern void __gcov_indirect_call_profiler_v2 (gcov_type, void *);
+extern void __gcov_indirect_call_profiler_v2_atomic (gcov_type, void *);
extern void __gcov_time_profiler (gcov_type *);
extern void __gcov_average_profiler (gcov_type *, gcov_type);
extern void __gcov_ior_profiler (gcov_type *, gcov_type);
--
2.9.2
