This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add counter histogram to fdo summary (issue6465057)
On Wed, Aug 29, 2012 at 6:12 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> 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).
gcc_checking_assert isn't available, since tsystem.h not system.h is
included. I could probably just remove the assert (to be safe,
silently return if i is out of bounds?).
>
> 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 (.
Ok.
>> + }
>> + }
>> +}
>> +
>> /* 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?
Ok, removing that check and I am switching the histogram merging code
to handle the case where there are different numbers of counters. It
will end up with the same number of counters as in the summary we are
merging into since that is the num we keep above when runs > 0 to
start with.
>> + /* 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?
Sure, I will do that instead.
>> +/* Merge SRC_HISTO into TGT_HISTO. */
>
> Perhaps comment about overall concept of the merging routine would suit here.
Ok.
>> -#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.
>From my understanding of the mode attribute meanings, which I thought
are defined in terms of the number of smallest addressable units, the
code in gcov-io.h that sets up the gcov_type typedef will always end
up with a gcov_type that is 32 or 64 bits? I.e. when BITS_PER_UNIT is
8 it will use either SI or DI which will end up either 32 or 64, and
when BITS_PER_UNIT is 16 it would use either HI or SI which would
again be either 32 or 64. Is that wrong and we can end up with a 16
bit gcov_type?
The GCOV_TYPE_SIZE was being defined everywhere except when IN_GOV (so
it was being defined IN_LIBGCOV), but I wanted it defined
unconditionally because I am using it to determine the required number
of histogram entries.
In any case, I think it will be more straightforward to define the
number of histogram entries based on the max possible gcov_type size
which is 64 (so 256 entries). This will make implementing the bit mask
simpler, since it will always require the same number of gcov_unsigned
ints (8).
>
> Patch is OK if it passed profiledbootstrap modulo the comments above.
Ok, thanks. Working on the fixes above.
Teresa
> Thanks!
> Honza
--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413