Re: [GOOGLE] Iterative AutoFDO

[Xinliang David Li <davidxl at google dot com>]

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