On 11/7/18 5:49 PM, Indu Bhagat wrote:
> I have been looking at -fdump-ipa-profile dump with an intention to sanitize
>
> bits of information so that one may use it to judge the "quality of a profile"
>
> in FDO.
>
> The overall question I want to address is - are there ways to know which
> functions were not run in the training run, i.e. have ZERO profile ?
> (This patch corrects some dumped info; in a subsequent patch I would like to
> add
>
> some more explicit information/diagnostics.)
>
> Towards that end, I noticed that there are a couple of misleading bits of
> information (so I think) in the symbol table dump listing all functions in the
>
> compilation unit :
> --- "globally 0" appears even when profile data has not been fed by
> feedback
>
> profile (not the intent as the documentation of
> profile_guessed_global0
>
> in profile-count.h suggests).
> --- "unlikely_executed" should appear only when there is profile feedback
> or
>
> a function attribute is specified (as per documentation of
> node_frequency in coretypes.h). "unlikely_executed" in case of STALE or
>
> NO profile is misleading in my opinion.
>
> Summary of changes :
>
> 1. This patch makes some adjustments around how x_profile_status of a function
>
> is set - x_profile_status should be set to PROFILE_READ only when there is a
>
> profile for a function read from the .gcda file. So, instead of relying on
> profile_info (set whenever the gcda feedback file is present, even if the
> function does not have a profile available in the file), use exec_counts
> (non null when function has a profile (the latter may or may not be zero)). In
>
> essence, x_profile_status and profile_count::m_quality
> are set consistent to the stated intent (in code comments.)
>
> 2. A minor change in coverage.c is for more precise location of the message
>
> Following -fdump-ipa-profile dump excerpts show the effect :
>
> ------------------------------------------------
> -O1, -O2, -O3
> ------------------------------------------------
>
> 0. APPLICABLE PROFILE
> Trunk : Function flags: count:224114269 body hot
> After Patch : Function flags: count:224114269 (precise) body hot
>
> 1. STALE PROFILE
> (i.e., those cases covered by Wcoverage-mismatch; when control flow changes
> between profile-generate and profile-use)
> Trunk : Function flags: count:224114269 body hot
> After Patch : Function flags: count:224114269 (precise) body hot
>
> 2. NO PROFILE
> (i.e., those cases covered by Wmissing-profile; when function has no profile
>
> available in the .gcda file)
> Trunk (missing .gcda file) : Function flags: count:1073741824 (estimated
> locally) body
>
> Trunk (missing function) : Function flags: count: 1073741824 (estimated
> locally, globally 0) body unlikely_executed
>
> After Patch (missing .gcda file) : Function flags: count:1073741824
> (estimated locally) body
>
> After Patch (missing function) : Function flags: count:1073741824 (estimated
> locally) body
>
>
> 3. ZERO PROFILE (functions not run in training run)
> Trunk : Function flags: count: 1073741824 (estimated locally, globally 0)
> body unlikely_executed
>
> After Patch (remains the same) : count: 1073741824 (estimated locally,
> globally 0) body unlikely_executed
>
>
> --------------------------------------------------
> O0
> --------------------------------------------------
> In O0, flag_guess_branch_prob is not set. This makes the profile_quality set
> to
>
> (precise) for most of the above cases.
>
> 0. APPLICABLE PROFILE
> Trunk : Function flags: count:224114269 body hot
> After Patch : Function flags: count:224114269 (precise) body hot
>
> 1. STALE PROFILE
> (i.e., those cases covered by Wcoverage-mismatch; when control flow changes
> between profile-generate and profile-use)
> Trunk : Function flags: count:224114269 body hot
> After Patch : Function flags: count:224114269 (precise) body hot
>
> 2. NO PROFILE
> (i.e., those cases covered by Wmissing-profile; when function has no profile
>
> available in the .gcda file)
> Trunk (missing file) : Function flags: body
> Trunk (missing function) : Function flags: count:0 body unlikely_executed
> After Patch (missing file) : Function flags: body
> *** After Patch (missing function) : Function flags: count:0 (precise) body
> (*** This remains misleading, and I do not have a solution for this; as use
> of heuristics
>
> to guess branch probability is not allowed in O0)
>
> 3. ZERO PROFILE (functions not run in training run)
> Trunk : Function flags: count:0 body unlikely_executed
> After Patch : Function flags: count:0 (precise) body
>
> --------------------------------------------------
>
> make check-gcc on x86_64 shows no new failures.
>
> (A related PR was https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86957 where we
> added diagnostics for the NO PROFILE case.)
>
>
>
> precise-ipa-dump-optinfo.patch.ver1
>
> diff --git a/gcc/coverage.c b/gcc/coverage.c
> index 599a3bb..7595e6c 100644
> --- a/gcc/coverage.c
> +++ b/gcc/coverage.c
> @@ -358,7 +358,7 @@ get_coverage_counts (unsigned counter, unsigned
> cfg_checksum,
> if (warning_printed && dump_enabled_p ())
> {
> dump_user_location_t loc
> - = dump_user_location_t::from_location_t (input_location);
> + = dump_user_location_t::from_function_decl (current_function_decl);
> dump_printf_loc (MSG_MISSED_OPTIMIZATION, loc,
> "use -Wno-error=coverage-mismatch to tolerate "
> "the mismatch but performance may drop if the "
Patches should include a ChangeLog entry. Because your patch didn't
include one, it's unclear if this hunk is something you really intended
to submit or not. What's the point behind going from using
input_location to the function decl?
> diff --git a/gcc/profile.c b/gcc/profile.c
> index 2130319..57e3f3c 100644
> --- a/gcc/profile.c
> +++ b/gcc/profile.c
> @@ -698,6 +698,11 @@ compute_branch_probabilities (unsigned cfg_checksum,
> unsigned lineno_checksum)
> }
> }
>
> + if (exec_counts)
> + {
> + profile_status_for_fn (cfun) = PROFILE_READ;
> + }
> +
Extraneous curly braces. In general if you have a single statement,
don't bother with curly braces.
> @@ -705,7 +710,7 @@ compute_branch_probabilities (unsigned cfg_checksum,
> unsigned lineno_checksum)
> bb->count = profile_count::from_gcov_type (bb_gcov_count (bb));
> /* If function was not trained, preserve local estimates including
> statically
> determined zero counts. */
> - else
> + else if (profile_status_for_fn (cfun) == PROFILE_READ)
> FOR_ALL_BB_FN (bb, cfun)
> if (!(bb->count == profile_count::zero ()))
> bb->count = bb->count.global0 ();
So I'm guessing the point here is to only set the count to global0 when
we read in profile data for the function and the profile data didn't
have any hits for the function. Right?
> @@ -1317,12 +1327,12 @@ branch_prob (void)
> values.release ();
> free_edge_list (el);
> coverage_end_function (lineno_checksum, cfg_checksum);
> - if (flag_branch_probabilities && profile_info)
> + if (flag_branch_probabilities
> + && (profile_status_for_fn (cfun) == PROFILE_READ))
So what's the point of this change? Under what circumstances do the two
conditionals do something different. I don't know this code well, but
ISTM that if profile_info is nonzero, then the profile status is going
to be PROFILE_READ. Is that not the case?
Jeff
