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
Related show

Commit Message

Jan Hubicka Jan. 28, 2020, 8:40 p.m.
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. | #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,

>

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,