> 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?
>
> 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...
> 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?
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;
>
