Fine grained AutoFDO implementation

Fine grained AutoFDO implementation

[davidxl, 2012-10-14 05:50:59]

Have not finished reading. Here is the first batch of comments.

David

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode502
gcc/auto-profile.c:502: get_inline_stack_size_by_stmt (gimple stmt)
It might be better to rename the function to reflect the fact that it is getting
the inline stack of the profile_collection_binary -- may be
get_prof_collect_inline_stack_by .. and make the comment more explicit.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode646
gcc/auto-profile.c:646: (*entry_count) += func_entry->entry_count;
+= is not neede --> it is initialized to 0.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode653
gcc/auto-profile.c:653: {
Remove { }

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode654
gcc/auto-profile.c:654: (*entry_count) += func_entry->entry_count;
+= --> =

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode668
gcc/auto-profile.c:668: node->count += entry_count;
Why +=?

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode905
gcc/auto-profile.c:905: afdo_get_callsite_count (struct cgraph_edge *edge,
gcov_type *total_count,
Why just total count? Why not making 'entry' count and max count (for the inline
instance) also available?

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraph.c
File gcc/cgraph.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraph.c#newcode2194
gcc/cgraph.c:2194: new_node->clone = n->clone;
This code needs more explanation and comments.

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraphunit.c
File gcc/cgraphunit.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraphunit.c#newcode1352
gcc/cgraphunit.c:1352: if (flag_auto_profile)
Add comments about why this is delayed and split from the profile read (it is
documented in the wiki).

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c
File gcc/ipa-inline-transform.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c#n...
gcc/ipa-inline-transform.c:208: /* we do not have enough information to
calculate the node count
Fix indentation.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c#n...
gcc/ipa-inline-transform.c:393: afdo_add_copy_scale (e);
Why recording the scale?

https://codereview.appspot.com/6567079/diff/2001/gcc/ira-int.h
File gcc/ira-int.h (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ira-int.h#newcode49
gcc/ira-int.h:49: ? REG_FREQ_MAX : (freq * REG_FREQ_MAX / BB_FREQ_MAX)			   \
More comment here.

[davidxl, 2012-10-15 02:31:28]

comment batch 2.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode906
gcc/auto-profile.c:906: bool is_total)
DOCUMENT TOTAL_COUNT, IS_TOTAL.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c
File gcc/ipa-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode485
gcc/ipa-inline.c:485: if (afdo_get_callsite_count (edge, &callsite_total_count,
true)
Add comment about 'true' and 'false' here -- total/non-total.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode486
gcc/ipa-inline.c:486: && maybe_hot_count_p (callsite_total_count))
Do you need to use a separate parameter to control autoFDO hotness?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode491
gcc/ipa-inline.c:491: }
Does autoFDO need to fall through to the next check?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode873
gcc/ipa-inline.c:873: if (afdo_get_callsite_count (edge, &callsite_count,
false))
Why can't the edge->count be computed during callgraph rebuild?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode891
gcc/ipa-inline.c:891: relative_time_benefit (callee_info, edge, time_growth)) /
growth;
How sensitive is the performance to the badness computation here?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode1683
gcc/ipa-inline.c:1683: if (callee->global.inlined_to)// || 1)
Typo ?

https://codereview.appspot.com/6567079/diff/2001/gcc/loop-unroll.c
File gcc/loop-unroll.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/loop-unroll.c#newcode1158
gcc/loop-unroll.c:1158: && expected_loop_iterations (loop) >
loop->header->count)
The logic here is not quite clear. Please add comment.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-cfg.c
File gcc/tree-cfg.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-cfg.c#newcode1816
gcc/tree-cfg.c:1816: 
Does the count merging happen for regular FDO too?

[dehao, 2012-10-15 21:21:35]

Hi, David,

I've integrated your review, and I'll update the gcc wiki to add more stuff
about scaling to it.

Thanks,
Dehao

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode502
gcc/auto-profile.c:502: get_inline_stack_size_by_stmt (gimple stmt)
On 2012/10/14 05:51:00, davidxl wrote:
> It might be better to rename the function to reflect the fact that it is
getting
> the inline stack of the profile_collection_binary -- may be
> get_prof_collect_inline_stack_by .. and make the comment more explicit.

Emm, but this function is to get inline stack at the optimized IR. We don't have
a function to get inline stack for profile_collection_binary.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode646
gcc/auto-profile.c:646: (*entry_count) += func_entry->entry_count;
On 2012/10/14 05:51:00, davidxl wrote:
> += is not neede --> it is initialized to 0.

This is needed because we have all sorts of .isra functions, in which case we'd
want to combine their profiles into one. I've added a comment to specify this.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode653
gcc/auto-profile.c:653: {
On 2012/10/14 05:51:00, davidxl wrote:
> Remove { }

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode654
gcc/auto-profile.c:654: (*entry_count) += func_entry->entry_count;
On 2012/10/14 05:51:00, davidxl wrote:
> += --> =

Same as above.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode668
gcc/auto-profile.c:668: node->count += entry_count;
On 2012/10/14 05:51:00, davidxl wrote:
> Why +=?

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode905
gcc/auto-profile.c:905: afdo_get_callsite_count (struct cgraph_edge *edge,
gcov_type *total_count,
On 2012/10/14 05:51:00, davidxl wrote:
> Why just total count? Why not making 'entry' count and max count (for the
inline
> instance) also available?

This would be hard because in many cases, the entry block is simply optimized
away when inlined.

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode906
gcc/auto-profile.c:906: bool is_total)
On 2012/10/15 02:31:28, davidxl wrote:
> DOCUMENT TOTAL_COUNT, IS_TOTAL.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraph.c
File gcc/cgraph.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraph.c#newcode2194
gcc/cgraph.c:2194: new_node->clone = n->clone;
On 2012/10/14 05:51:00, davidxl wrote:
> This code needs more explanation and comments.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraphunit.c
File gcc/cgraphunit.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/cgraphunit.c#newcode1352
gcc/cgraphunit.c:1352: if (flag_auto_profile)
On 2012/10/14 05:51:00, davidxl wrote:
> Add comments about why this is delayed and split from the profile read (it is
> documented in the wiki).

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c
File gcc/ipa-inline-transform.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c#n...
gcc/ipa-inline-transform.c:208: /* we do not have enough information to
calculate the node count
On 2012/10/14 05:51:00, davidxl wrote:
> Fix indentation.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline-transform.c#n...
gcc/ipa-inline-transform.c:393: afdo_add_copy_scale (e);
On 2012/10/14 05:51:00, davidxl wrote:
> Why recording the scale?

Emm, this is the tricky part. I will add that to the gcc wiki too. Basically,
when we annotate the body of inlined functions, we have 3 options:

1. reading directly from profile
2. scaling directly from original count
3. reading from profile and then scale

The afdo_add_copy_scale is mainly designed for the 3rd case.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c
File gcc/ipa-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode485
gcc/ipa-inline.c:485: if (afdo_get_callsite_count (edge, &callsite_total_count,
true)
On 2012/10/15 02:31:28, davidxl wrote:
> Add comment about 'true' and 'false' here -- total/non-total.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode486
gcc/ipa-inline.c:486: && maybe_hot_count_p (callsite_total_count))
On 2012/10/15 02:31:28, davidxl wrote:
> Do you need to use a separate parameter to control autoFDO hotness?

Right now, the original threshold works well. Basically, because autoFDO uses
total counts instead of max count, using the same threshold means that
not-so-hot callees may get higher priority than it normally do. However, for
AutoFDO use case, we'd still prefer to inline those not-so-hot callsites because
if it is not inlined, the profiles will be wasted. To make things worse, if it
is not inlined, it may not have profile at all because we do not do profile
merging.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode491
gcc/ipa-inline.c:491: }
On 2012/10/15 02:31:28, davidxl wrote:
> Does autoFDO need to fall through to the next check?

I think hot caller heuristic is still important for AutoFDO, isn't it?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode873
gcc/ipa-inline.c:873: if (afdo_get_callsite_count (edge, &callsite_count,
false))
On 2012/10/15 02:31:28, davidxl wrote:
> Why can't the edge->count be computed during callgraph rebuild?

Because during ipa-inline, we need to have valid edge counts to make inline
decisions.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode891
gcc/ipa-inline.c:891: relative_time_benefit (callee_info, edge, time_growth)) /
growth;
On 2012/10/15 02:31:28, davidxl wrote:
> How sensitive is the performance to the badness computation here?

It should be sensitive when inlining threshold can be met, otherwise it should
be fine. I'll run some experiments to evaluate the difference.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode1683
gcc/ipa-inline.c:1683: if (callee->global.inlined_to)// || 1)
On 2012/10/15 02:31:28, davidxl wrote:
> Typo ?

Emm, could be, I'll retest this.

https://codereview.appspot.com/6567079/diff/2001/gcc/ira-int.h
File gcc/ira-int.h (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ira-int.h#newcode49
gcc/ira-int.h:49: ? REG_FREQ_MAX : (freq * REG_FREQ_MAX / BB_FREQ_MAX)			   \
On 2012/10/14 05:51:00, davidxl wrote:
> More comment here.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/loop-unroll.c
File gcc/loop-unroll.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/loop-unroll.c#newcode1158
gcc/loop-unroll.c:1158: && expected_loop_iterations (loop) >
loop->header->count)
On 2012/10/15 02:31:28, davidxl wrote:
> The logic here is not quite clear. Please add comment.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-cfg.c
File gcc/tree-cfg.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-cfg.c#newcode1816
gcc/tree-cfg.c:1816: 
On 2012/10/15 02:31:28, davidxl wrote:
> Does the count merging happen for regular FDO too?

No, it does not. We do this because AutoFDO profile is not as accurate as FDO.
However, adding this for FDO should not harm performance.

[davidxl, 2012-10-17 05:09:38]

in predict.c (this file is not loaded properly)

-      loop_optimizer_finalize ();
+      if (flag_auto_profile && counts_to_freqs ())
+	{
+	  afdo_calculate_branch_prob ();

Why does it need to recompute branch prob? Are they incrementally updated just
like regular FDO?

+	  counts_to_freqs();   

Why is counts_to_freqs() called again?

+	  profile_status = PROFILE_READ;
+	  compute_function_frequency ();
+	}
+      else
+	{
+	  loop_optimizer_init (0);
+	  add_noreturn_fake_exit_edges ();
+	  mark_irreducible_loops ();
+	  connect_infinite_loops_to_exit ();
+	  estimate_bb_frequencies ();
+	  remove_fake_exit_edges ();
+	  loop_optimizer_finalize ();
+	}
     }
   else if (profile_status == PROFILE_READ)
-    counts_to_freqs ();
+    {
+      if (flag_auto_profile)
+	afdo_calculate_branch_prob ();

autoFDO together with regular FDO?


+      counts_to_freqs ();
+    }

[davidxl, 2012-10-17 06:14:26]

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c
File gcc/tree-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1836
gcc/tree-inline.c:1836: }
so it means if the same inline happens in the profile-collection binary, use
that instance's profile count. Otherwise use the scaled count.

This needs some comments.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1933
gcc/tree-inline.c:1933: 
why clearing the flag?

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode2269
gcc/tree-inline.c:2269: count_scale = 0;
Why setting count_scale -> 0?

[dehao, 2012-10-17 18:37:03]

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c
File gcc/tree-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1836
gcc/tree-inline.c:1836: }
On 2012/10/17 06:14:26, davidxl wrote:
> so it means if the same inline happens in the profile-collection binary, use
> that instance's profile count. Otherwise use the scaled count.
> 
> This needs some comments.

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1933
gcc/tree-inline.c:1933: 
On 2012/10/17 06:14:26, davidxl wrote:
> why clearing the flag?

Because:

1. in the following line, it checks if flags is NULL.
2. we will recalculate the edge->flags later when inlines are done.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode2269
gcc/tree-inline.c:2269: count_scale = 0;
On 2012/10/17 06:14:26, davidxl wrote:
> Why setting count_scale -> 0?

This is tricky, consider the following case: foo() calls bar() with call count
of 0 (sample count), and bar()'s entry count is also 0 (sample count), but
bar()'s body is really hot. In this case, how should we scale? Because we have
no other info here, we can only choose either scale it as 0% or 100%. I picked
0% because I've observed cases choosing 100% is harmful (the callee's large bb
counts will make caller's bb frequency 0). Of cause, this could also cause
problems, but we need to pick either way.

[dehao, 2012-10-17 18:43:39]

not sure why this shows "bad content"...

On 2012/10/17 05:09:38, davidxl wrote:
> in predict.c (this file is not loaded properly)
> 
> -      loop_optimizer_finalize ();
> +      if (flag_auto_profile && counts_to_freqs ())
> +	{
> +	  afdo_calculate_branch_prob ();
> 
> Why does it need to recompute branch prob? Are they incrementally updated just
> like regular FDO?

Because FDO uses edge count to rebuild WCFG, which is a deterministic problem.
But in AutoFDO, we use bb count to rebuild WCFG, which is non-deterministic, and
we'd need to propagate along the CFG to estimate the edge frequency.

Concerning cost, as it is only invoked for functions with profile count, and is
only invoked once after ipa-inlining (for each function that has profiles), it
should not be too much overhead.

Comments added in the code.
> 
> +	  counts_to_freqs();   
> 
> Why is counts_to_freqs() called again?

The first call just check if the current function has profiles. Then we'd need
to do the propagation first, and recalculate the frequency based on propagated
profile counts.

> 
> +	  profile_status = PROFILE_READ;
> +	  compute_function_frequency ();
> +	}
> +      else
> +	{
> +	  loop_optimizer_init (0);
> +	  add_noreturn_fake_exit_edges ();
> +	  mark_irreducible_loops ();
> +	  connect_infinite_loops_to_exit ();
> +	  estimate_bb_frequencies ();
> +	  remove_fake_exit_edges ();
> +	  loop_optimizer_finalize ();
> +	}
>      }
>    else if (profile_status == PROFILE_READ)
> -    counts_to_freqs ();
> +    {
> +      if (flag_auto_profile)
> +	afdo_calculate_branch_prob ();
> 
> autoFDO together with regular FDO?

As it is guarded by flag_auto_profile, regular FDO will not do it.
> 
> 
> +      counts_to_freqs ();
> +    }

[davidxl, 2012-10-17 18:50:55]

On Wed, Oct 17, 2012 at 11:43 AM,  <dehao@google.com> wrote:
> not sure why this shows "bad content"...

The review tool is buggy ..

>
>
> On 2012/10/17 05:09:38, davidxl wrote:
>>
>> in predict.c (this file is not loaded properly)
>
>
>> -      loop_optimizer_finalize ();
>> +      if (flag_auto_profile && counts_to_freqs ())
>> +       {
>> +         afdo_calculate_branch_prob ();
>
>
>> Why does it need to recompute branch prob? Are they incrementally
>
> updated just
>>
>> like regular FDO?
>
>
> Because FDO uses edge count to rebuild WCFG, which is a deterministic
> problem. But in AutoFDO, we use bb count to rebuild WCFG, which is
> non-deterministic, and we'd need to propagate along the CFG to estimate
> the edge frequency.
>
> Concerning cost, as it is only invoked for functions with profile count,
> and is only invoked once after ipa-inlining (for each function that has
> profiles), it should not be too much overhead.
>
> Comments added in the code.
>
>
>> +         counts_to_freqs();
>
>
>> Why is counts_to_freqs() called again?
>
>
> The first call just check if the current function has profiles. Then
> we'd need to do the propagation first, and recalculate the frequency
> based on propagated profile counts.
>
>
>
>> +         profile_status = PROFILE_READ;
>> +         compute_function_frequency ();
>> +       }
>> +      else
>> +       {
>> +         loop_optimizer_init (0);
>> +         add_noreturn_fake_exit_edges ();
>> +         mark_irreducible_loops ();
>> +         connect_infinite_loops_to_exit ();
>> +         estimate_bb_frequencies ();
>> +         remove_fake_exit_edges ();
>> +         loop_optimizer_finalize ();
>> +       }
>>       }
>>     else if (profile_status == PROFILE_READ)
>> -    counts_to_freqs ();
>> +    {
>> +      if (flag_auto_profile)
>> +       afdo_calculate_branch_prob ();
>
>
>> autoFDO together with regular FDO?
>
>
> As it is guarded by flag_auto_profile, regular FDO will not do it.

My question is do  we expect to use afdo and regular FDO together?

David


>
>
>
>> +      counts_to_freqs ();
>> +    }
>
>
>
>
> https://codereview.appspot.com/6567079/

[dehao, 2012-10-17 20:02:38]

On Wed, Oct 17, 2012 at 11:50 AM, Xinliang David Li <davidxl@google.com>wrote:

> On Wed, Oct 17, 2012 at 11:43 AM,  <dehao@google.com> wrote:
> > not sure why this shows "bad content"...
>
> The review tool is buggy ..
>
> >
> >
> > On 2012/10/17 05:09:38, davidxl wrote:
> >>
> >> in predict.c (this file is not loaded properly)
> >
> >
> >> -      loop_optimizer_finalize ();
> >> +      if (flag_auto_profile && counts_to_freqs ())
> >> +       {
> >> +         afdo_calculate_branch_prob ();
> >
> >
> >> Why does it need to recompute branch prob? Are they incrementally
> >
> > updated just
> >>
> >> like regular FDO?
> >
> >
> > Because FDO uses edge count to rebuild WCFG, which is a deterministic
> > problem. But in AutoFDO, we use bb count to rebuild WCFG, which is
> > non-deterministic, and we'd need to propagate along the CFG to estimate
> > the edge frequency.
> >
> > Concerning cost, as it is only invoked for functions with profile count,
> > and is only invoked once after ipa-inlining (for each function that has
> > profiles), it should not be too much overhead.
> >
> > Comments added in the code.
> >
> >
> >> +         counts_to_freqs();
> >
> >
> >> Why is counts_to_freqs() called again?
> >
> >
> > The first call just check if the current function has profiles. Then
> > we'd need to do the propagation first, and recalculate the frequency
> > based on propagated profile counts.
> >
> >
> >
> >> +         profile_status = PROFILE_READ;
> >> +         compute_function_frequency ();
> >> +       }
> >> +      else
> >> +       {
> >> +         loop_optimizer_init (0);
> >> +         add_noreturn_fake_exit_edges ();
> >> +         mark_irreducible_loops ();
> >> +         connect_infinite_loops_to_exit ();
> >> +         estimate_bb_frequencies ();
> >> +         remove_fake_exit_edges ();
> >> +         loop_optimizer_finalize ();
> >> +       }
> >>       }
> >>     else if (profile_status == PROFILE_READ)
> >> -    counts_to_freqs ();
> >> +    {
> >> +      if (flag_auto_profile)
> >> +       afdo_calculate_branch_prob ();
> >
> >
> >> autoFDO together with regular FDO?
> >
> >
> > As it is guarded by flag_auto_profile, regular FDO will not do it.
>
> My question is do  we expect to use afdo and regular FDO together?
>

Nope. But AutoFDO will also make profile_status as PROFILE_READ.

Dehao


>
> David
>
>
> >
> >
> >
> >> +      counts_to_freqs ();
> >> +    }
> >
> >
> >
> >
> > https://codereview.appspot.com/6567079/
>

[davidxl, 2012-10-22 21:04:58]

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode646
gcc/auto-profile.c:646: (*entry_count) += func_entry->entry_count;
But it is always reset to 0 at the begining of this function, so what is the
effect here?

On 2012/10/15 21:21:35, dehao wrote:
> On 2012/10/14 05:51:00, davidxl wrote:
> > += is not neede --> it is initialized to 0.
> 
> This is needed because we have all sorts of .isra functions, in which case
we'd
> want to combine their profiles into one. I've added a comment to specify this.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c
File gcc/ipa-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode486
gcc/ipa-inline.c:486: && maybe_hot_count_p (callsite_total_count))
On 2012/10/15 21:21:35, dehao wrote:
> On 2012/10/15 02:31:28, davidxl wrote:
> > Do you need to use a separate parameter to control autoFDO hotness?
> 
> Right now, the original threshold works well. Basically, because autoFDO uses
> total counts instead of max count, using the same threshold means that
> not-so-hot callees may get higher priority than it normally do. However, for
> AutoFDO use case, we'd still prefer to inline those not-so-hot callsites
because
> if it is not inlined, the profiles will be wasted. To make things worse, if it
> is not inlined, it may not have profile at all because we do not do profile
> merging.

Ok. Have you done experiment with different threshold with autoFDO?

Also what is the main cause for the remaining perf gap between autoFDO and
regular one?

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode491
gcc/ipa-inline.c:491: }
Ok. Add a comment at the end of the autofdo block to indicate this is by design.

On 2012/10/15 21:21:35, dehao wrote:
> On 2012/10/15 02:31:28, davidxl wrote:
> > Does autoFDO need to fall through to the next check?
> 
> I think hot caller heuristic is still important for AutoFDO, isn't it?

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c
File gcc/tree-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1933
gcc/tree-inline.c:1933: 
I wonder if there are other cases where the empty flag is assumed by the code
but not satisfied due to EDGE_ANNOTATE.


On 2012/10/17 18:37:03, dehao wrote:
> On 2012/10/17 06:14:26, davidxl wrote:
> > why clearing the flag?
> 
> Because:
> 
> 1. in the following line, it checks if flags is NULL.
> 2. we will recalculate the edge->flags later when inlines are done.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode2269
gcc/tree-inline.c:2269: count_scale = 0;
On 2012/10/17 18:37:03, dehao wrote:
> On 2012/10/17 06:14:26, davidxl wrote:
> > Why setting count_scale -> 0?
> 
> This is tricky, consider the following case: foo() calls bar() with call count
> of 0 (sample count), and bar()'s entry count is also 0 (sample count), but
> bar()'s body is really hot. In this case, how should we scale? Because we have
> no other info here, we can only choose either scale it as 0% or 100%. I picked
> 0% because I've observed cases choosing 100% is harmful (the callee's large bb
> counts will make caller's bb frequency 0). Of cause, this could also cause
> problems, but we need to pick either way.

How about 50% ?

[dehao, 2012-10-22 23:48:53]

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/auto-profile.c#newcode646
gcc/auto-profile.c:646: (*entry_count) += func_entry->entry_count;
Yes, you are right. The merging was done in process_auto_profile. So the +=
should be replace by =.
On 2012/10/22 21:04:58, davidxl wrote:
> But it is always reset to 0 at the begining of this function, so what is the
> effect here?
> 
> On 2012/10/15 21:21:35, dehao wrote:
> > On 2012/10/14 05:51:00, davidxl wrote:
> > > += is not neede --> it is initialized to 0.
> > 
> > This is needed because we have all sorts of .isra functions, in which case
> we'd
> > want to combine their profiles into one. I've added a comment to specify
this.
>

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c
File gcc/ipa-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode486
gcc/ipa-inline.c:486: && maybe_hot_count_p (callsite_total_count))
On 2012/10/22 21:04:58, davidxl wrote:
> On 2012/10/15 21:21:35, dehao wrote:
> > On 2012/10/15 02:31:28, davidxl wrote:
> > > Do you need to use a separate parameter to control autoFDO hotness?
> > 
> > Right now, the original threshold works well. Basically, because autoFDO
uses
> > total counts instead of max count, using the same threshold means that
> > not-so-hot callees may get higher priority than it normally do. However, for
> > AutoFDO use case, we'd still prefer to inline those not-so-hot callsites
> because
> > if it is not inlined, the profiles will be wasted. To make things worse, if
it
> > is not inlined, it may not have profile at all because we do not do profile
> > merging.
> 
> Ok. Have you done experiment with different threshold with autoFDO?

Not yet. I'll experiment and update the results later.

> 
> Also what is the main cause for the remaining perf gap between autoFDO and
> regular one? 

I've added some new tuning in the patch:

1. Add workset support in profile (enable Teresa's unroll work)
2. New heuristic to set update_original when clone a cgraph_node (so that we
will not miss hot functions when profile is not accurate.
3. Move hot comdat functions to a separate section so that the code layout will
be better.

Now AutoFDO performance can reach an average of 93% of FDO. And for large
applications, the gap is even closer (it even beats FDO on some benchmarks by
33%). I'll spend some more time to do performance analysis and update the result
in the doc.

https://codereview.appspot.com/6567079/diff/2001/gcc/ipa-inline.c#newcode491
gcc/ipa-inline.c:491: }
On 2012/10/22 21:04:58, davidxl wrote:
> Ok. Add a comment at the end of the autofdo block to indicate this is by
design.
> 
> On 2012/10/15 21:21:35, dehao wrote:
> > On 2012/10/15 02:31:28, davidxl wrote:
> > > Does autoFDO need to fall through to the next check?
> > 
> > I think hot caller heuristic is still important for AutoFDO, isn't it?
> 

Done.

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c
File gcc/tree-inline.c (right):

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode1933
gcc/tree-inline.c:1933: 
that's a good point, I found another place in cfg.c and modified it to output
this flag when dumping the IR.

On 2012/10/22 21:04:58, davidxl wrote:
> I wonder if there are other cases where the empty flag is assumed by the code
> but not satisfied due to EDGE_ANNOTATE.
> 
> 
> On 2012/10/17 18:37:03, dehao wrote:
> > On 2012/10/17 06:14:26, davidxl wrote:
> > > why clearing the flag?
> > 
> > Because:
> > 
> > 1. in the following line, it checks if flags is NULL.
> > 2. we will recalculate the edge->flags later when inlines are done.
>

https://codereview.appspot.com/6567079/diff/2001/gcc/tree-inline.c#newcode2269
gcc/tree-inline.c:2269: count_scale = 0;
I'll do some experiments to compare.

Another option is to still set the count_scale to 0, and in ipa-inline, we don't
allow this type of callsite to inline (disable hot-caller heuristic for
callsites whose callee has 0 entry count but non-zero max_bb_count.

On 2012/10/22 21:04:58, davidxl wrote:
> On 2012/10/17 18:37:03, dehao wrote:
> > On 2012/10/17 06:14:26, davidxl wrote:
> > > Why setting count_scale -> 0?
> > 
> > This is tricky, consider the following case: foo() calls bar() with call
count
> > of 0 (sample count), and bar()'s entry count is also 0 (sample count), but
> > bar()'s body is really hot. In this case, how should we scale? Because we
have
> > no other info here, we can only choose either scale it as 0% or 100%. I
picked
> > 0% because I've observed cases choosing 100% is harmful (the callee's large
bb
> > counts will make caller's bb frequency 0). Of cause, this could also cause
> > problems, but we need to pick either way.
> 
> How about 50% ?
>

[davidxl, 2012-11-16 23:39:12]

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.c
File gcc/auto-profile.c (right):

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.c#newcode2
gcc/auto-profile.c:2: Copyright (C) 2011. Free Software Foundation, Inc.
2011 --> 2012

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.c#newcode1015
gcc/auto-profile.c:1015: /*      inform (0, "Vesion number does not mathch.");
typo.

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.c#newcode1016
gcc/auto-profile.c:1016: flag_auto_profile = 0;
Commented out?

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.c#newcode1657
gcc/auto-profile.c:1657: 
Document return value.

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.h
File gcc/auto-profile.h (right):

https://codereview.appspot.com/6567079/diff/29001/gcc/auto-profile.h#newcode3
gcc/auto-profile.h:3: Free Software Foundation, Inc.
The copyright date should be 2012