[PATCH] Drop profile reproducibility flag as it is not used.

Jan Hubicka hubicka@ucw.cz
Fri Jan 22 14:51:12 GMT 2021


> 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?
> -	  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.

OK with that change, thanks a lot!
Honza


More information about the Gcc-patches mailing list