On Wed, Jul 16, 2014 at 4:42 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> Instrumentation based FDO is designed to work when the source files
>> that are used to generate the instr binary match exactly with the
>> sources in profile-use compile. It is known historically that using
>> stale profile (due to source changes, not gcda format change) can lead
>> to lots of mismatch warnings and even worse -- compiler ICEs. This is
>> due to two reasons:
>> 1) the profile lookup for each function is based on funcdef_no which
>> can change when function definition order is changed or new functions
>> are inserted in the middle of a source
>> 2) the indirect call target id may change due to source changes:
>> before GCC4.9, the id uses cgraph uid which is as bad as funcdef_no.
>> Attributing wrong IC target to the indirect call site is the main
>> cause of compiler ICE (we have signature match check, but bad target
>> can leak through result in problem later). Starting from gcc49, the
>> indirect target profiling uses profile_id which is stable for public
>> functions.
>
> We should not ICE however when the targets gets wrong. There is some basic
> type checking on the place, do you have testcase where we still ICE?
I don't have test cases at hand (when it happened, it was not
considered important to fix due to the stale profile used by the
user). The basic check may have some holes - e.g. related to vararg
functions.
>>
>> This patch introduces a new parameter for FDO to determine whether to
>> use internal id or assembler name based external id for profile
>> lookup. When the external id is used, GCC FDO will become very
>> tolerant to simple source changes.
>>
>> Note that autoFDO solves this problem but it is currently limited to
>> Intel platforms with LBR support.
>>
>> (Tested with SPEC, SPEC06 and large internal benchmarks. No performance
>> impact).
>>
>> Ok for trunk?
>
> I wonder if there are any downsides for using this always?
> We still compare checksums so we should warn user that profile is out of date,
> so I would consistently switch from funcdef_no to profile_id...
I wonder about the same. The tests show no downside.
>
>> Index: coverage.c
>> ===================================================================
>> --- coverage.c (revision 212682)
>> +++ coverage.c (working copy)
>> @@ -54,6 +54,7 @@ along with GCC; see the file COPYING3.
>> #include "intl.h"
>> #include "filenames.h"
>> #include "target.h"
>> +#include "params.h"
>>
>> #include "gcov-io.h"
>> #include "gcov-io.c"
>> @@ -369,8 +370,10 @@ get_coverage_counts (unsigned counter, u
>> da_file_name);
>> return NULL;
>> }
>> -
>> - elt.ident = current_function_funcdef_no + 1;
>> + if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
>> + elt.ident = current_function_funcdef_no + 1;
>> + else
>> + elt.ident = coverage_compute_profile_id (cgraph_get_node (cfun->decl));
>> elt.ctr = counter;
>> entry = counts_hash->find (&elt);
>> if (!entry || !entry->summary.num)
>> @@ -416,7 +419,8 @@ get_coverage_counts (unsigned counter, u
>> }
>> else if (entry->lineno_checksum != lineno_checksum)
>> {
>> - warning (0, "source locations for function %qE have changed,"
>> + warning (OPT_Wcoverage_mismatch,
>> + "source locations for function %qE have changed,"
>> " the profile data may be out of date",
>> DECL_ASSEMBLER_NAME (current_function_decl));
>> }
>> @@ -581,12 +585,13 @@ coverage_compute_profile_id (struct cgra
>> {
>> expanded_location xloc
>> = expand_location (DECL_SOURCE_LOCATION (n->decl));
>> + bool use_name_only = (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID) ==
>> 0);
>>
>> - chksum = xloc.line;
>> + chksum = (use_name_only ? 0 : xloc.line);
>> chksum = coverage_checksum_string (chksum, xloc.file);
>> chksum = coverage_checksum_string
>> (chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl)));
>> - if (first_global_object_name)
>> + if (!use_name_only && first_global_object_name)
>
> I think this will cause troubles with static functions and LTO indirect call
> optimization. We really want to make two static functions with same name to
> have
> different IDs when they come from different units.
>
> I see why you do not like first_global_object_name because changing it would
> cause
> all functions from that unit to drop the profiles. Perhaps we can combine
> function name
> and compilation unit (gcov file) name?
that is a good idea -- it will also solve the LTO problem you mentioned above.
Will update the patch.
David
>
> Honza
>
>> chksum = coverage_checksum_string
>> (chksum, first_global_object_name);
>> chksum = coverage_checksum_string
>> @@ -645,7 +650,12 @@ coverage_begin_function (unsigned lineno
>>
>> /* Announce function */
>> offset = gcov_write_tag (GCOV_TAG_FUNCTION);
>> - gcov_write_unsigned (current_function_funcdef_no + 1);
>> + if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
>> + gcov_write_unsigned (current_function_funcdef_no + 1);
>> + else
>> + gcov_write_unsigned (coverage_compute_profile_id (
>> + cgraph_get_node (current_function_decl)));
>> +
>> gcov_write_unsigned (lineno_checksum);
>> gcov_write_unsigned (cfg_checksum);
>> gcov_write_string (IDENTIFIER_POINTER
>> @@ -682,8 +692,13 @@ coverage_end_function (unsigned lineno_c
>> if (!DECL_EXTERNAL (current_function_decl))
>> {
>> item = ggc_alloc<coverage_data> ();
>> -
>> - item->ident = current_function_funcdef_no + 1;
>> +
>> + if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
>> + item->ident = current_function_funcdef_no + 1;
>> + else
>> + item->ident = coverage_compute_profile_id (
>> + cgraph_get_node (cfun->decl));
>> +
>> item->lineno_checksum = lineno_checksum;
>> item->cfg_checksum = cfg_checksum;
>>
>
