Re: FDO usability patch -- cfg and lineno checksum

[Xinliang David Li <davidxl@google.com>]

On Wed, Apr 27, 2011 at 8:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Please don't forget about changelogs..
>> Index: tree.c
>> ===================================================================
>> --- tree.c    (revision 172802)
>> +++ tree.c    (working copy)
>> @@ -8513,14 +8513,12 @@ dump_tree_statistics (void)
>
> The crc bits was already reviewed, right?

Yes.

>> -       else if (entry->checksum != checksum)
>> +       else if (entry->lineno_checksum != lineno_checksum
>> +                || entry->cfg_checksum != cfg_checksum)
>>           {
>>             error ("coverage mismatch for function %u while reading execution counters",
>>                    fn_ident);
>> -           error ("checksum is %x instead of %x", entry->checksum, checksum);
>> +           error ("checksum is (%x,%x) instead of (%x,%x)",
>> +                  entry->lineno_checksum, entry->cfg_checksum,
>> +                  lineno_checksum, cfg_checksum);
>
> Can't we give more informative message whether code changes or it seems to be
> optimization options disprepancy?

Good idea -- but to change the warning not the error here. For the
warning (which is promoted to error by default) currently it is:

error: coverage mismatch for function xxxx while reading counter yyy.
 note: control flow checksum is aaa instead of bbb

Could be:
error: function xxx's control flow does match its profile data (counter yyy).
  note: use -Wno-error=coverage-mismatch to tolerate the mismatch but
performance may drop if the function is hot.

>> +unsigned
>> +coverage_compute_cfg_checksum (void)
>> +{
>> +  basic_block bb;
>> +  unsigned chksum = n_basic_blocks;
>> +
>> +  FOR_EACH_BB (bb)
>> +    {
>> +      edge e;
>> +      edge_iterator ei;
> Perhaps also
> chksum = crc32_byte (chksum, bb->index)
> for cases where destination stays the same, but source of edge moves (i.e. with forwarder
> blocks)

Yes.

Thanks,

David

>
> Patch is OK, thanks
> Honza
>