[GOOGLE] Iterative AutoFDO

[Xinliang David Li]

 > /* Program behavior changed, original promoted (and inlined) target is not
 > hot any more. Will avoid promote the original target.  */
 > if (total >= info->first * 0.5)
 >   return false;

This part is still not clear: Difference between OLD_INFO and INFO,
factor 0.5 comes from where etc.

> gcov_type autofdo_source_profile::get_callsite_total_count (
>    struct cgraph_edge *edge) const

Formatting and missing documentation -- there are other cases like
this in the source.

>     /* No need to promoted the stmt if its in promoted_stmts (means
>       it is already been promoted in the previous iterations).  */

Make it clear that ic promotion and early-inline-2 is done in multiple
iterations.

> This is needed because an indirect call might be promoted and

might have been

 >    inlined in the profiled binary. If we do not promote and inline
 >    these indirect calls before annothation, the profile for these

annothation --> annotation.

>   for (int i = 0; i < PARAM_VALUE (PARAM_EARLY_INLINER_MAX_ITERATIONS); i++)
>   {
>     if (!flag_value_profile_transformations
>         || !autofdo::afdo_vpt_for_early_inline (&promoted_stmts))
>      break;
>    early_inliner ();

Does it leave any indirect call sites not promoted after this loop? In
other words, is there a need to keep the value profile transformation
after the cfg annotation?

A related question -- for indirect callsites that are not
promoted/inlined in the profiled binary, will indirect call promotion
and inlining before cfg annotation cause problems? The inline instance
won't find profile anymore.

David

On Mon, Oct 7, 2013 at 11:49 AM, Dehao Chen <dehao@google.com> wrote:
> Hi, David,
>
> Thanks for the review. Patch updated.
>
> Dehao
>
> On Sun, Oct 6, 2013 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Adding additional early inline + value transform (calling them as
>> utility functions) is 'unconventional' way of invoking passes. It
>> would be helpful to do more heavy documentation by providing more
>> examples and explaining why simply augmenting the indirect target info
>> for promoted icall site with inlined call stack profile data and rely
>> on existing value profile transform to kick in is not enough.
>>
>> The patch also misses documentation for many functions at the
>> definition site. There are also a couple of coding style problems such
>> as function name does not start a new line (but following return
>> type).
>>
>>
>>
>>> /* Return the combined relative location for STMT.  */
>>
>>> static unsigned
>>> get_relative_location_for_stmt (gimple stmt)
>>> {
>>
>> More explanation on 'relative' -- it is not mentioned in other places either.
>>
>>
>>> /* Program behavior changed, original promoted (and inlined) target is not
>>>  hot any more. Will avoid promote the original target.  */
>>>  if (total >= info->first * 0.5)
>>>    return false;
>>
>> This needs more explanation.
>>
>>> /* For a given BB, return its execution count, and annotate value profile
>>>   if a stmt is not in PROMOTED. Add the location of annotated stmt to
>>>   ANNOTATED.  */
>>
>> More explanation on why skipping promoted ones.
>>
>>  >      || promoted_stmts->find (stmt) != promoted_stmts->end ())
>>  >       continue;
>>  >     promoted_stmts->insert (stmt);
>>
>> It is not quite clear how 'promoted' stmts are identified here.
>>
>>>     /* First do indirect call promotion and early inline to make the
>>>     IR match the profiled binary before actual annotation.  */
>>>      autofdo::stmt_set promoted_stmts;
>>>      for (int i = 0; i < PARAM_VALUE (PARAM_EARLY_INLINER_MAX_ITERATIONS); i++)
>>>    {
>>>      early_inliner ();
>>>      if (!flag_value_profile_transformations
>>>          || !autofdo::afdo_vpt_for_early_inline (&promoted_stmts))
>>>        break;
>>>    }'
>>
>> This needs heavy documentation. ALso why putting early inline before
>> value transform?
>>
>> David
>>
>>
>>
>>
>> On Mon, Sep 23, 2013 at 10:51 AM, Dehao Chen <dehao@google.com> wrote:
>>> This patch fixes the issue of indirect call promotion while using
>>> AutoFDO optimized binary to collect profile, and use the new profile
>>> to re-optimize the binary. Before the patch, the indirect call is
>>> promoted (and likely inlined) in the profiled binary, and will not be
>>> promoted in the new iteration. With the patch, a new iterative
>>> vpt+einline pass is added before annotation to make sure hot promoted
>>> indirect calls are still promoted before annotation.
>>>
>>> Bootstrapped and passed regression test and benchmark test.
>>>
>>> OK for google-4_8 branch?
>>>
>>> Thanks,
>>> Dehao
>>>
>>> http://codereview.appspot.com/13492045