This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [GOOGLE] Iterative AutoFDO


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

Attachment: patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]