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  <hubi...@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,


Reply via email to