[PATCH] minor FDO profile related fixes

[PATCH] minor FDO profile related fixes

[Indu Bhagat]

[-- Attachment #1: Type: text/plain, Size: 4607 bytes --]

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.)


[-- Attachment #2: precise-ipa-dump-optinfo.patch.ver1 --]
[-- Type: text/plain, Size: 2976 bytes --]

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 "
diff --git a/gcc/profile-count.c b/gcc/profile-count.c
index f4ab244..90f4feb 100644
--- a/gcc/profile-count.c
+++ b/gcc/profile-count.c
@@ -83,6 +83,8 @@ profile_count::dump (FILE *f) const
 	fprintf (f, " (auto FDO)");
       else if (m_quality == profile_guessed)
 	fprintf (f, " (guessed)");
+      else if (m_quality == profile_precise)
+	fprintf (f, " (precise)");
     }
 }
 
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;
+    }
+
   /* If we have real data, use them!  */
   if (bb_gcov_count (ENTRY_BLOCK_PTR_FOR_FN (cfun))
       || !flag_guess_branch_prob)
@@ -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 ();
@@ -718,6 +723,11 @@ compute_branch_probabilities (unsigned cfg_checksum, unsigned lineno_checksum)
 
   if (dump_file)
     {
+      fprintf (dump_file, " Profile feedback for function");
+      fprintf (dump_file, ((profile_status_for_fn (cfun) == PROFILE_READ)
+			   ? " is available \n"
+			   : " is not available \n"));
+
       fprintf (dump_file, "%d branches\n", num_branches);
       if (num_branches)
 	for (i = 0; i < 10; i++)
@@ -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))
     {
       struct loop *loop;
       if (dump_file && (dump_flags & TDF_DETAILS))
 	report_predictor_hitrates ();
-      profile_status_for_fn (cfun) = PROFILE_READ;
 
       /* At this moment we have precise loop iteration count estimates.
 	 Record them to loop structure before the profile gets out of date. */

Re: [PATCH] minor FDO profile related fixes

[Jeff Law]

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

Re: [PATCH] minor FDO profile related fixes

[Indu Bhagat]

[-- Attachment #1: Type: text/plain, Size: 3839 bytes --]

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


[-- Attachment #2: Changelog.precise-ipa-dump-optinfo.patch.ver1 --]
[-- Type: text/plain, Size: 489 bytes --]

gcc/ChangeLog:

2018-11-07  "Indu Bhagat"  <"indu.bhagat@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.

Re: [PATCH] minor FDO profile related fixes

[Martin Liška]

On 11/8/18 1:49 AM, 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.)

Hi.

Thanks for the patch. I'm not a maintainer, but the idea of the patch looks correct to me.
One question about adding "(precise)", have you verified that the patch can survive regression
tests?

Thanks,
Martin

> 
> 
> 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 "
> diff --git a/gcc/profile-count.c b/gcc/profile-count.c
> index f4ab244..90f4feb 100644
> --- a/gcc/profile-count.c
> +++ b/gcc/profile-count.c
> @@ -83,6 +83,8 @@ profile_count::dump (FILE *f) const
>  	fprintf (f, " (auto FDO)");
>        else if (m_quality == profile_guessed)
>  	fprintf (f, " (guessed)");
> +      else if (m_quality == profile_precise)
> +	fprintf (f, " (precise)");
>      }
>  }
>  
> 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;
> +    }
> +
>    /* If we have real data, use them!  */
>    if (bb_gcov_count (ENTRY_BLOCK_PTR_FOR_FN (cfun))
>        || !flag_guess_branch_prob)
> @@ -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 ();
> @@ -718,6 +723,11 @@ compute_branch_probabilities (unsigned cfg_checksum, unsigned lineno_checksum)
>  
>    if (dump_file)
>      {
> +      fprintf (dump_file, " Profile feedback for function");
> +      fprintf (dump_file, ((profile_status_for_fn (cfun) == PROFILE_READ)
> +			   ? " is available \n"
> +			   : " is not available \n"));
> +
>        fprintf (dump_file, "%d branches\n", num_branches);
>        if (num_branches)
>  	for (i = 0; i < 10; i++)
> @@ -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))
>      {
>        struct loop *loop;
>        if (dump_file && (dump_flags & TDF_DETAILS))
>  	report_predictor_hitrates ();
> -      profile_status_for_fn (cfun) = PROFILE_READ;
>  
>        /* At this moment we have precise loop iteration count estimates.
>  	 Record them to loop structure before the profile gets out of date. */
> 

Re: [PATCH] minor FDO profile related fixes

[Indu Bhagat]

On 11/12/2018 01:48 AM, Martin Liška wrote:
>> make check-gcc on x86_64 shows no new failures.
>>
>> (A related PR washttps://gcc.gnu.org/bugzilla/show_bug.cgi?id=86957  where we added diagnostics for the NO PROFILE case.)
> Hi.
>
> Thanks for the patch. I'm not a maintainer, but the idea of the patch looks correct to me.
> One question about adding "(precise)", have you verified that the patch can survive regression
> tests?
>
> Thanks,
> Martin
>

make check-gcc (configured with --enable-languages=c,c++,fortran,go,lto) 
shows no new failures.
make -k check also looks ok.

Re: [PATCH] minor FDO profile related fixes

[Martin Liška]

On 11/16/18 7:08 AM, Indu Bhagat wrote:
> 
> On 11/12/2018 01:48 AM, Martin Liška wrote:
>>> make check-gcc on x86_64 shows no new failures.
>>>
>>> (A related PR washttps://gcc.gnu.org/bugzilla/show_bug.cgi?id=86957  where we added diagnostics for the NO PROFILE case.)
>> Hi.
>>
>> Thanks for the patch. I'm not a maintainer, but the idea of the patch looks correct to me.
>> One question about adding "(precise)", have you verified that the patch can survive regression
>> tests?
>>
>> Thanks,
>> Martin
>>
> 
> make check-gcc (configured with --enable-languages=c,c++,fortran,go,lto) shows no new failures.
> make -k check also looks ok.

Thanks for testing.

Honza can you please review that?
Martin

Re: [PATCH] minor FDO profile related fixes

[Jeff Law]

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.bhagat@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