On 1/25/21 4:06 PM, H.J. Lu wrote:
On Mon, Jan 25, 2021 at 4:54 AM Martin Liška <mli...@suse.cz> wrote:

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.


This breaks GCC bootstrap:

../../src-master/gcc/value-prof.c: In function ??bool
get_nth_most_common_value(gimple*, const char*, histogram_value,
gcov_type*, gcov_type*, gcov_type*, unsigned int)??:
../../src-master/gcc/value-prof.c:780:29: error: ISO C++11 does not
support the ??q?? gnu_printf length modifier [-Werror=format=]
   780 |         fprintf (dump_file, "Histogram value dropped in %qs mode",
       |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src-master/gcc/value-prof.c:780:59: error: use of ??q?? length
modifier with ??s?? type character has either no effect or undefined
behavior [-Werror=format=]
   780 |         fprintf (dump_file, "Histogram value dropped in %qs mode",
       |                                                           ^
../../src-master/gcc/value-prof.c:788:29: error: ISO C++11 does not
support the ??q?? gnu_printf length modifier [-Werror=format=]
   788 |         fprintf (dump_file, "Histogram value dropped in %qs mode",
       |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src-master/gcc/value-prof.c:788:59: error: use of ??q?? length
modifier with ??s?? type character has either no effect or undefined
behavior [-Werror=format=]
   788 |         fprintf (dump_file, "Histogram value dropped in %qs mode",
       |



Oh, sorry for that.

Fixed in the current master.

Martin

Reply via email to