[PATCH 2/4] Implement N disk counters for single value and indirect call counters.

Jan Hubicka hubicka@ucw.cz
Thu Jun 6 12:50:00 GMT 2019


> 
> gcc/ChangeLog:
> 
> 2019-06-04  Martin Liska  <mliska@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  <mliska@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



More information about the Gcc-patches mailing list