[PATCH] D148876: [IndirectCallPromotion] Clear value profile metadata after the last run of indirect-call-promotion in a pipeline

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

In D148876#4289051 <https://reviews.llvm.org/D148876#4289051>, @hoy wrote:

> I see. Merging could cause the loss of flow-sensitivity for PGO, but not merging doesn't give optimal code quality.
>
> Here are my thoughts about what could happen with @mingmingl's approach that allows merging only after ICP.

For instrumentation PGO, the situation is much simpler -- just need to make sure the callsite merge does not happen before instrumentation. Your concern is on AutoFDO with iteration.

> 1. For the training build that does not use a profile, my intuition is that we will always merge the indirect callsites, which will lead to a merged !VP profile.

For initial build of the profiling binary, yes this is true.

> 2. Then for the next build that uses the profile, the callsites will first be promoted in separate, and then the leftover unpromoted callsites will be merged.

Depending the phase ordering of sample loading. The unpromoted callsites is not merged before sample loading, then I assume one of the callsites will be annotated with VP data (assuming that in the profile binary the merged callsite inherits one source's debug info). If that is the case, this patch will prevent those sites from being merged.  And starting from this iteration, all future iterations should be fine (remain unmerged).

> That merged callsite will lead to a merged !VP profile. The already promoted callsites, if not merged, will lead to separate !VP profiles.

The promoted callsite should behave similarly as unpromoted callsite.

For instance,

original code:

  if (cond) {
      fp1->foo();   //line 3; targeting bar
  } else  {
    fp2-> foo(); // line 5; targeting blah

}

> merging in profile binary
===========================

fp = select cond, fp1, fp2;
 fp->foo();    // line 3 targeting bar, blah

> After sampling, line 3 VP data 'bar', 'blah'
----------------------------------------------

> In optimized build, assuming merge does not happen before sample loading,
---------------------------------------------------------------------------

    1. fp1 gets VP data 'bar', and 'blah' and get promoted,
    2. fp2 at line 5 has no VP data
  1. merging will be prevented
  2. line 3 will be promoted (less precisedly)
1. line5 won't be promoted (no VP data)

> in the next round of profiling, both line 3 and line 5 have independent VP data, and things will start get stable from now on.
--------------------------------------------------------------------------------------------------------------------------------

> 3. For all subsequent builds, the separate !VP profiles will be dominant, the leftover merged profile shouldn't be important as they represent non-typical paths.

The VP data from promoted branches will be mapped to the same line including the left over one.

For instance, the line 3 is promoted:

   if (fp1 == 'bar') 
       bar();
   else if (fp1 == 'blah')
       blah();            // no samples in this case
  else
    fp1();

The resulting profile for line 3 will have 'bar' as dominating target.

> The questions is, in #2, whether promoted callsites can be merged or not. I'm not quite sure but since their promotion is based on the same profile from #1, the resulting direct calls will all look the same which might be good candidates to merge. If merged, the generated profile based on this build will still lose flow-sensitivity.

Icall promotion itself will likely block any future merging.

> There's also another possibility which is no callsites can be merged after ICP,  because of the conditional code introduced during ICP.  If this is common, then it sounds like just letting indirect callsites be merged (#1) as is isn't a deal breaker.

See above. This patch will help stablize promotion behavior after 2 iterations which is good for AFDO.

> Thoughts?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148876/new/

https://reviews.llvm.org/D148876