Changelog entry attached. Sorry about that.
Comments inline.
Thanks
On 11/09/2018 04:23 PM, Jeff Law wrote:
On 11/7/18 5:49 PM, Indu Bhagat wrote:
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?
This is intended to be a part of the patch. The patch intends to make opt-info
and dumps related to ipa-profile more precise.
Using from_function_decl gives the precise location of the function.
from_location_t gives the same incorrect location for each function
with the coverage-mismatch issue in a compilation unit (location appears to be
the end of file)
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.
I will correct this.
@@ -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?
Yes, correct.
(Longer answer : Without this more selective conditional, count is set
to global0 even for functions which were added
between profile-generate and profile-use.)
@@ -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?
On Trunk, there is a direct relationship between the three :
1. (.gcda file) is present --implies--> 2. profile_info set
--implies--> 3. profile_status set to PROFILE_READ
But for functions added between profile-generate and profile-use, 3.
should not be done based on 2.
For such functions, profile_info may be set (because there is a .gcda
file), but profile_status should not be set
to PROFILE_READ, because such functions have no feedback data.
Jeff
gcc/ChangeLog:
2018-11-07 "Indu Bhagat" <"indu.bha...@oracle.com">
* coverage.c (get_coverage_counts): Use from_function_decl for precise
function location.
* profile-count.c (profile_count::dump): Add handling for precise
profile quality.
* profile.c (compute_branch_probabilities): Rely on exec_counts instead
of profile_info to set x_profile_status of function.
(branch_prob): Do not set x_profile_status of function based on
profile_info. Done above based on exec_counts.
