Re: [PATCH] Add counter histogram to fdo summary (issue6465057)

[Jan Hubicka <hubicka at ucw dot cz>]

> Index: libgcc/libgcov.c
> ===================================================================
> --- libgcc/libgcov.c	(revision 190736)
> +++ libgcc/libgcov.c	(working copy)
> @@ -276,6 +276,78 @@ gcov_version (struct gcov_info *ptr, gcov_unsigned
>    return 1;
>  }
>  
> +/* Insert counter VALUE into HISTOGRAM.  */
> +
> +static void
> +gcov_histogram_insert(gcov_bucket_type *histogram, gcov_type value)
> +{
> +  unsigned i;
> +
> +  i = gcov_histo_index(value);
> +  gcc_assert (i < GCOV_HISTOGRAM_SIZE);
Does checking_assert work in libgcov? I do not think internal consistency check
should go to --enable-checking=release libgcov. We want to maintain it as
lightweight as possible. (I see there are two existing gcc_asserts, since they
report file format corruption, I think they should give better diagnostic).

Inliner will do good job here, but perhaps explicit inline fits.
> +      for (f_ix = 0; f_ix != gi_ptr->n_functions; f_ix++)
> +        {
> +          gfi_ptr = gi_ptr->functions[f_ix];
> +
> +          if (!gfi_ptr || gfi_ptr->key != gi_ptr)
> +            continue;
> +
> +          ci_ptr = &gfi_ptr->ctrs[ctr_info_ix];
> +          for (ix = 0; ix < ci_ptr->num; ix++)
> +            gcov_histogram_insert(cs_ptr->histogram, ci_ptr->values[ix]);
Space before (.
> +        }
> +    }
> +}
> +
>  /* Dump the coverage counts. We merge with existing counts when
>     possible, to avoid growing the .da files ad infinitum. We use this
>     program's checksum to make sure we only accumulate whole program
> @@ -347,6 +419,7 @@ gcov_exit (void)
>  	    }
>  	}
>      }
> +  gcov_compute_histogram (&this_prg);
> @@ -598,11 +669,18 @@ gcov_exit (void)
>  	  if (gi_ptr->merge[t_ix])
>  	    {
>  	      if (!cs_prg->runs++)
> -		cs_prg->num = cs_tprg->num;
> +                cs_prg->num = cs_tprg->num;
> +              else if (cs_prg->num != cs_tprg->num)
> +                goto read_mismatch;

Doesn't think check that all the programs that contain this unit are the same?
I.e. will this survive profiledbootstrap where we interleave cc1 and cc1plus?
> +  /* Count number of non-zero histogram entries. The histogram is only
> +     currently computed for arc counters.  */
> +  csum = &summary->ctrs[GCOV_COUNTER_ARCS];
> +  for (h_ix = 0; h_ix < GCOV_HISTOGRAM_SIZE; h_ix++)
> +    {
> +      if (csum->histogram[h_ix].num_counters > 0)
> +        h_cnt++;
> +    }
> +  gcov_write_tag_length (tag, GCOV_TAG_SUMMARY_LENGTH(h_cnt));
>    gcov_write_unsigned (summary->checksum);
>    for (csum = summary->ctrs, ix = GCOV_COUNTERS_SUMMABLE; ix--; csum++)
>      {
> @@ -380,6 +388,21 @@ gcov_write_summary (gcov_unsigned_t tag, const str
>        gcov_write_counter (csum->sum_all);
>        gcov_write_counter (csum->run_max);
>        gcov_write_counter (csum->sum_max);
> +      if (ix != GCOV_COUNTER_ARCS)
> +        {
> +          gcov_write_unsigned (0);
> +          continue;
> +        }
> +      gcov_write_unsigned (h_cnt);
> +      for (h_ix = 0; h_ix < GCOV_HISTOGRAM_SIZE; h_ix++)
> +        {
> +          if (!csum->histogram[h_ix].num_counters)
> +            continue;
> +          gcov_write_unsigned (h_ix);

It is kind of waste to write whole unsigned for each histogram index.
What about writting bitmap of non-zero entries followed by each entry?
> +/* Merge SRC_HISTO into TGT_HISTO.  */

Perhaps comment about overall concept of the merging routine would suit here.
> -#else /*!IN_GCOV */
> -#define GCOV_TYPE_SIZE (LONG_LONG_TYPE_SIZE > 32 ? 64 : 32)

Why do you need t omove this out of !libgcov? I do not thing this is correct for all configurations.
i.e. gcov_type may be 16bit.

Patch is OK if it passed profiledbootstrap modulo the comments above.
Thanks!
Honza