>
> gcc/ChangeLog:
>
> 2019-06-04 Martin Liska <mli...@suse.cz>
>
> * gcov-io.h (GCOV_DISK_SINGLE_VALUES): New.
> (GCOV_SINGLE_VALUE_COUNTERS): Likewise.
> * ipa-profile.c (ipa_profile_generate_summary):
> Use get_most_common_single_value.
> * tree-profile.c (gimple_init_gcov_profiler):
> Instrument with __gcov_one_value_profiler_v2
> and __gcov_indirect_call_profiler_v4.
> * value-prof.c (dump_histogram_value):
> Print all values for HIST_TYPE_SINGLE_VALUE.
> (stream_in_histogram_value): Set number of
> counters for HIST_TYPE_SINGLE_VALUE.
> (get_most_common_single_value): New.
> (gimple_divmod_fixed_value_transform):
> Use get_most_common_single_value.
> (gimple_ic_transform): Likewise.
> (gimple_stringops_transform): Likewise.
> (gimple_find_values_to_profile): Set number
> of counters for HIST_TYPE_SINGLE_VALUE.
> * value-prof.h (get_most_common_single_value):
> New.
>
> libgcc/ChangeLog:
>
> 2019-06-04 Martin Liska <mli...@suse.cz>
>
> * Makefile.in: Add __gcov_one_value_profiler_v2 and
> __gcov_indirect_call_profiler_v4.
> * libgcov-merge.c (__gcov_merge_single): Change
> function signature.
> (merge_single_value_set): New.
> * libgcov-profiler.c (__gcov_one_value_profiler_body):
> Do not update counters[2].
> (__gcov_one_value_profiler): Remove.
> (__gcov_one_value_profiler_atomic): Rename to ...
> (__gcov_one_value_profiler_v2): ... this.
> (__gcov_indirect_call_profiler_v3): Rename to ...
> (__gcov_indirect_call_profiler_v4): ... this.
> * libgcov.h (__gcov_one_value_profiler): Remove.
> (__gcov_one_value_profiler_atomic): Remove.
> (__gcov_indirect_call_profiler_v3): Remove.
> (__gcov_one_value_profiler_v2): New.
> (__gcov_indirect_call_profiler_v4): New.
> ---
> gcc/gcov-io.h | 7 +++
> gcc/ipa-profile.c | 13 +++--
> gcc/tree-profile.c | 9 ++-
> gcc/value-prof.c | 120 ++++++++++++++++++++------------------
> gcc/value-prof.h | 2 +
> libgcc/Makefile.in | 5 +-
> libgcc/libgcov-merge.c | 77 ++++++++++++++++--------
> libgcc/libgcov-profiler.c | 43 +++-----------
> libgcc/libgcov.h | 5 +-
> 9 files changed, 147 insertions(+), 134 deletions(-)
>
> diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
> index f2cf4047579..008a1271979 100644
> --- a/gcc/tree-profile.c
> +++ b/gcc/tree-profile.c
> @@ -165,10 +165,9 @@ gimple_init_gcov_profiler (void)
> = build_function_type_list (void_type_node,
> gcov_type_ptr, gcov_type_node,
> NULL_TREE);
> - fn_name = concat ("__gcov_one_value_profiler", fn_suffix, NULL);
> - tree_one_value_profiler_fn = build_fn_decl (fn_name,
> - one_value_profiler_fn_type);
> - free (CONST_CAST (char *, fn_name));
> + tree_one_value_profiler_fn
> + = build_fn_decl ("__gcov_one_value_profiler_v2",
> + one_value_profiler_fn_type);
Why you no longer need the optional atomic suffix here?
> diff --git a/gcc/value-prof.c b/gcc/value-prof.c
> index 1e14e532070..e893ca084c9 100644
> --- a/gcc/value-prof.c
> +++ b/gcc/value-prof.c
> @@ -259,15 +259,19 @@ dump_histogram_value (FILE *dump_file, histogram_value
> hist)
> break;
>
> case HIST_TYPE_SINGLE_VALUE:
> - fprintf (dump_file, "Single value ");
> + case HIST_TYPE_INDIR_CALL:
> + fprintf (dump_file, (hist->type == HIST_TYPE_SINGLE_VALUE
> + ? "Single values " : "Indirect call "));
> if (hist->hvalue.counters)
> {
> - fprintf (dump_file, "value:%" PRId64
> - " match:%" PRId64
> - " wrong:%" PRId64,
> - (int64_t) hist->hvalue.counters[0],
> - (int64_t) hist->hvalue.counters[1],
> - (int64_t) hist->hvalue.counters[2]);
> + for (unsigned i = 0; i < GCOV_DISK_SINGLE_VALUES; i++)
> + {
> + fprintf (dump_file, "[%" PRId64 ":%" PRId64 "]",
> + (int64_t) hist->hvalue.counters[2 * i],
> + (int64_t) hist->hvalue.counters[2 * i + 1]);
> + if (i != GCOV_DISK_SINGLE_VALUES - 1)
> + fprintf (dump_file, ", ");
> + }
Unless there are some compelling reasons, I would still include in dump what is
meaning of the values - not everyone is fluent in that ;)
> @@ -758,23 +779,19 @@ gimple_divmod_fixed_value_transform
> (gimple_stmt_iterator *si)
> if (!histogram)
> return false;
>
> + if (!get_most_common_single_value (histogram, &val, &count))
> + return false;
> +
> value = histogram->hvalue.value;
> - val = histogram->hvalue.counters[0];
> - count = histogram->hvalue.counters[1];
> - all = histogram->hvalue.counters[2];
> + all = gimple_bb (stmt)->count.ipa ().to_gcov_type ();
Here it is safe to use gimple_bb (stmt)->count.ipa ().to_gcov_type () under an
assumptions that profile is consistent - I am not sure how often one gets
mismatches with -fprofile-correction at multithreaded programs, maybe it would
be worth to check before we drop the code logging it to dumps.
In ipa-profile it is not quite the case since we do CFG optimization and other
transforms between profile instrumentaiton and IPA passes. I would remember
probability within stmt histogram (like we do for speculative edges) that is
safe WRT BB profile updates.
So in your implementation we store 4 times as many vlaue counters in memory
during runtime? Originally I had in mind a variant where in memory everything
works as before, but in on-disk format one save the section for indirect
call counters multiple times implementing the update logic.
This should not be that hard do something:
for secno = 1...4
for every counter
read counter2 from disk
if (counter has non-zero exec count)
if counter2 is invalid
clear counter
else if counter and counter2 values match
increase counter2 hitrate
clear counter
else if counter2 has zero count
rewrite counter2 by counter
clear counter
else if secno==5
mark counter2 as invalid
clear ocunter
write counter2 to disk
So you do not really need to keep duplicates in memory at runtime.
You will still be able to tell if there was too many counters by checking
the last copy (instead of first as you do in your implementation)
> + value = gcov_get_counter_target ();
> + counter = gcov_get_counter ();
> +
> + if (counter == -1)
> + {
> + counters[1] = -1;
> + /* We can't return as we need to read all counters. */
> + continue;
> + }
> + else if (counter == 0 || counters[1] == -1)
> + {
> + /* We can't return as we need to read all counters. */
> + continue;
> + }
gcov_get_counter scales by gcov_get_merge_weight and thus it will
not reliably read -1 when you store -1
Honza
