On 1/22/21 3:51 PM, Jan Hubicka wrote:
gcc/ChangeLog:
PR gcov-profile/98739
* common.opt: Add missing sign symbol.
* value-prof.c (get_nth_most_common_value): Restore handling
of PROFILE_REPRODUCIBILITY_PARALLEL_RUNS and
PROFILE_REPRODUCIBILITY_MULTITHREADED.
libgcc/ChangeLog:
PR gcov-profile/98739
* libgcov-merge.c (__gcov_merge_topn): Mark when merging
ends with a dropped counter.
* libgcov.h (gcov_topn_add_value): Add return value.
---
gcc/common.opt | 2 +-
gcc/value-prof.c | 26 ++++++++++++++++++++------
libgcc/libgcov-merge.c | 11 ++++++++---
libgcc/libgcov.h | 13 +++++++++----
4 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/gcc/common.opt b/gcc/common.opt
index bde1711870d..a8a2b67a99d 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2248,7 +2248,7 @@ Enum(profile_reproducibility) String(parallel-runs)
Value(PROFILE_REPRODUCIBILIT
EnumValue
Enum(profile_reproducibility) String(multithreaded)
Value(PROFILE_REPRODUCIBILITY_MULTITHREADED)
-fprofile-reproducible
+fprofile-reproducible=
Common Joined RejectNegative Var(flag_profile_reproducible)
Enum(profile_reproducibility) Init(PROFILE_REPRODUCIBILITY_SERIAL)
-fprofile-reproducible=[serial|parallel-runs|multithreaded] Control level
of reproducibility of profile gathered by -fprofile-generate.
diff --git a/gcc/value-prof.c b/gcc/value-prof.c
index 4c916f4994f..3e899a39b84 100644
--- a/gcc/value-prof.c
+++ b/gcc/value-prof.c
@@ -747,8 +747,8 @@ gimple_divmod_fixed_value (gassign *stmt, tree value,
profile_probability prob,
abs (counters[0]) is the number of executions
for i in 0 ... TOPN-1
- counters[2 * i + 1] is target
- abs (counters[2 * i + 2]) is corresponding hitrate counter.
+ counters[2 * i + 2] is target
+ counters[2 * i + 3] is corresponding hitrate counter.
Value of counters[0] negative when counter became
full during merging and some values are lost. */
@@ -766,15 +766,29 @@ get_nth_most_common_value (gimple *stmt, const char
*counter_type,
*value = 0;
gcov_type read_all = abs_hwi (hist->hvalue.counters[0]);
+ gcov_type covered = 0;
+ for (unsigned i = 0; i < counters; ++i)
+ covered += hist->hvalue.counters[2 * i + 3];
gcov_type v = hist->hvalue.counters[2 * n + 2];
gcov_type c = hist->hvalue.counters[2 * n + 3];
if (hist->hvalue.counters[0] < 0
- && (flag_profile_reproducible == PROFILE_REPRODUCIBILITY_PARALLEL_RUNS
- || (flag_profile_reproducible
- == PROFILE_REPRODUCIBILITY_MULTITHREADED)))
- return false;
+ && flag_profile_reproducible == PROFILE_REPRODUCIBILITY_PARALLEL_RUNS)
+ {
+ if (dump_file)
+ fprintf (dump_file, "Histogram value dropped in %qs mode",
+ "-fprofile-reproducible=parallel-runs");
+ return false;
+ }
+ else if (covered != read_all
+ && flag_profile_reproducible ==
PROFILE_REPRODUCIBILITY_MULTITHREADED)
+ {
+ if (dump_file)
+ fprintf (dump_file, "Histogram value dropped in %qs mode",
+ "-fprofile-reproducible=multithreaded");
+ return false;
+ }
We can do that incrementally, but having opt-info=missed to print
warning when profile is rejected with info if it would be useful and how
frequent it is would make it easy for me to track this with firefox
builds.
It also seems a reasonable user facing feature - we could mention that
in documentation of profile-reproducibility flag and tell users they can
look for warnings about disabled transformations.
/* Indirect calls can't be verified. */
if (stmt
diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c
index 9306e8d688c..35936a8364b 100644
--- a/libgcc/libgcov-merge.c
+++ b/libgcc/libgcov-merge.c
@@ -107,7 +107,9 @@ __gcov_merge_topn (gcov_type *counters, unsigned n_counters)
gcov_type all = gcov_get_counter_ignore_scaling (-1);
gcov_type n = gcov_get_counter_ignore_scaling (-1);
- counters[GCOV_TOPN_MEM_COUNTERS * i] += all;
+ unsigned full = all < 0;
+ gcov_type *total = &counters[GCOV_TOPN_MEM_COUNTERS * i];
+ *total += full ? -all : all;
for (unsigned j = 0; j < n; j++)
{
@@ -115,9 +117,12 @@ __gcov_merge_topn (gcov_type *counters, unsigned
n_counters)
gcov_type count = gcov_get_counter_ignore_scaling (-1);
// TODO: we should use atomic here
Is the atomic possibly disasterou?
This one should be quite safe.
- gcov_topn_add_value (counters + GCOV_TOPN_MEM_COUNTERS * i, value,
- count, 0, 0);
+ full |= gcov_topn_add_value (counters + GCOV_TOPN_MEM_COUNTERS * i,
+ value, count, 0, 0);
}
+
+ if (full)
+ *total = -(*total);
Please add comment somewhere in __gcov_merge_topn what the sign bit is
useful for.
Done.
OK with that change, thanks a lot!
I've just installed the patch.
About the negative total value. Something similar can handle it:
diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
index df08e882dd7..ddc688509bd 100644
--- a/libgcc/libgcov.h
+++ b/libgcc/libgcov.h
@@ -443,7 +443,13 @@ gcov_topn_add_value (gcov_type *counters, gcov_type value,
gcov_type count,
int use_atomic, int increment_total)
{
if (increment_total)
- gcov_counter_add (&counters[0], 1, use_atomic);
+ {
+ /* In the multi-threaded mode, we can have an already merged profile
+ with a negative total value. In that case, we should bail out. */
+ if (counters[0] < 0)
+ return 0;
+ gcov_counter_add (&counters[0], 1, use_atomic);
+ }
struct gcov_kvp *prev_node = NULL;
struct gcov_kvp *minimal_node = NULL;
What do you think?
Another approach would be to have a global flag that will be checked in
libgcov-profiler.c.
But that will result in a slower code during instrumentation.
Martin
Honza
