On Mon, Jan 25, 2021 at 4:54 AM Martin Liška <[email protected]> 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",
|
--
H.J.
