On 11/9/18 7:12 PM, Indu Bhagat wrote:
> 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)
OK. And more generally we're trying to move away from input_location.
If associating the diagnostic with the function makes sense, then this
hunk is clearly OK.
>>> @@ -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
Right. That's the basic assumption I was working from.
>
> 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.
Ah, I see what you're getting at here. THe sources have changed since
we gathered the profiling data. In the case where new functions were
added to the source, we won't have profiling data for them. So we
should be checking for profiling data attached to a function. Got it.
>
>
> Changelog.precise-ipa-dump-optinfo.patch.ver1
>
> 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.
>
I'll fix up the trivial extraneous curley braces nit and commit this
momentarily.
THanks,
Jeff
