diff mbox series

Verify sanity of indirect call/topn profiles

Message ID 20200128204008.GF55295@kam.mff.cuni.cz
State New
Headers show
Series Verify sanity of indirect call/topn profiles | expand

Commit Message

Jan Hubicka Jan. 28, 2020, 8:40 p.m. UTC 
Hi,
I will try to reming this next stage1 since it is not regression fix.
I found it useful to have bit of sanity checking of the topn profiles to
work out the bugs in merging and updating that was there.

Honza

gcc/ChangeLog:

2020-01-28  Jan Hubicka  <hubicka@ucw.cz>

	* profile.c (compute_value_histograms): Verify profile sanity.

Comments

Martin Liška Jan. 29, 2020, 10:06 a.m. UTC | #1 
On 1/28/20 9:40 PM, Jan Hubicka wrote:
> Hi,
> I will try to reming this next stage1 since it is not regression fix.
> I found it useful to have bit of sanity checking of the topn profiles to
> work out the bugs in merging and updating that was there.

Hi.

Even though it's not a regression, I would like to see the patch
landing in GCC 10 release.

> 
> Honza
> 
> gcc/ChangeLog:
> 
> 2020-01-28  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* profile.c (compute_value_histograms): Verify profile sanity.
> 
> diff --git a/gcc/profile.c b/gcc/profile.c
> index cd754c4c66a..782534e5ab4 100644
> --- a/gcc/profile.c
> +++ b/gcc/profile.c
> @@ -856,10 +856,10 @@ compute_value_histograms (histogram_values values, unsigned cfg_checksum,
>         gimple_add_histogram_value (cfun, stmt, hist);
>         hist->hvalue.counters =  XNEWVEC (gcov_type, hist->n_counters);
>         for (j = 0; j < hist->n_counters; j++)
> -        if (aact_count)
> -          hist->hvalue.counters[j] = aact_count[j];
> -        else
> -          hist->hvalue.counters[j] = 0;
> +	if (aact_count)
> +	  hist->hvalue.counters[j] = aact_count[j];
> +	else
> +	  hist->hvalue.counters[j] = 0;

This should be skipped as it's only a formatting change?

>   
>         if (hist->type == HIST_TYPE_TOPN_VALUES
>   	  || hist->type == HIST_TYPE_INDIR_CALL)
> @@ -871,6 +871,26 @@ compute_value_histograms (histogram_values values, unsigned cfg_checksum,
>   		= RDIV (hist->hvalue.counters[2 * i + 2], GCOV_TOPN_VALUES);
>   
>   	  sort_hist_values (hist);
> +
> +	  /* Check profile sanity.  */
> +	  if (hist->hvalue.counters[2] != -1)
> +	    for (int i = 0; i < GCOV_TOPN_VALUES - 1 && ok; i++)
> +	      for (int j = i + 1; j < GCOV_TOPN_VALUES && ok; j++)

/home/marxin/Programming/gcc/gcc/profile.c: In function ‘void compute_value_histograms(histogram_values, unsigned int, unsigned int)’:
/home/marxin/Programming/gcc/gcc/profile.c:877:50: error: ‘ok’ was not declared in this scope
   877 |      for (int i = 0; i < GCOV_TOPN_VALUES - 1 && ok; i++)
       |                                                  ^~

> +		if ((hist->hvalue.counters[i * 2 + 1]
> +		     == hist->hvalue.counters[j * 2 + 1]
> +		     && hist->hvalue.counters[i * 2 + 2]
> +		     && hist->hvalue.counters[j * 2 + 2])

I know it's not important, but I would rather use:

+		     && hist->hvalue.counters[i * 2 + 2] > 0
+		     && hist->hvalue.counters[j * 2 + 2] > 0)

Thanks,
Martin

> +		    || hist->hvalue.counters[i * 2 + 2] < 0)
> +		  {
> +		    if (hist->type == HIST_TYPE_TOPN_VALUES)
> +		      error_at (gimple_location (stmt),
> +				"corrupted profile info:"
> +				" invalid topn profile histogram");
> +		    else
> +		      error_at (gimple_location (stmt),
> +				"corrupted profile info:"
> +				" indirect call profile histogram");
> +		  }
>   	}
>   
>         /* Time profiler counter is not related to any statement,
>
diff mbox series

Patch

diff --git a/gcc/profile.c b/gcc/profile.c
index cd754c4c66a..782534e5ab4 100644
--- a/gcc/profile.c
+++ b/gcc/profile.c
@@ -856,10 +856,10 @@  compute_value_histograms (histogram_values values, unsigned cfg_checksum,
       gimple_add_histogram_value (cfun, stmt, hist);
       hist->hvalue.counters =  XNEWVEC (gcov_type, hist->n_counters);
       for (j = 0; j < hist->n_counters; j++)
-        if (aact_count)
-          hist->hvalue.counters[j] = aact_count[j];
-        else
-          hist->hvalue.counters[j] = 0;
+	if (aact_count)
+	  hist->hvalue.counters[j] = aact_count[j];
+	else
+	  hist->hvalue.counters[j] = 0;
 
       if (hist->type == HIST_TYPE_TOPN_VALUES
 	  || hist->type == HIST_TYPE_INDIR_CALL)
@@ -871,6 +871,26 @@  compute_value_histograms (histogram_values values, unsigned cfg_checksum,
 		= RDIV (hist->hvalue.counters[2 * i + 2], GCOV_TOPN_VALUES);
 
 	  sort_hist_values (hist);
+
+	  /* Check profile sanity.  */
+	  if (hist->hvalue.counters[2] != -1)
+	    for (int i = 0; i < GCOV_TOPN_VALUES - 1 && ok; i++)
+	      for (int j = i + 1; j < GCOV_TOPN_VALUES && ok; j++)
+		if ((hist->hvalue.counters[i * 2 + 1]
+		     == hist->hvalue.counters[j * 2 + 1]
+		     && hist->hvalue.counters[i * 2 + 2]
+		     && hist->hvalue.counters[j * 2 + 2])
+		    || hist->hvalue.counters[i * 2 + 2] < 0)
+		  {
+		    if (hist->type == HIST_TYPE_TOPN_VALUES)
+		      error_at (gimple_location (stmt),
+				"corrupted profile info:"
+				" invalid topn profile histogram");
+		    else
+		      error_at (gimple_location (stmt),
+				"corrupted profile info:"
+				" indirect call profile histogram");
+		  }
 	}
 
       /* Time profiler counter is not related to any statement,