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

Fri Apr 21 16:50:54 PDT 2023

mingmingl 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.
>
> 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 an instrumentation + thinlto build, value profiles of two different indirect callsites won't be merged

- instrumentation of target values happens before the simplify-cfg pass that enables hoist and sink.
- https://gcc.godbolt.org/z/MM976vn55 is an example -> xur@ pointed out the counter index is from `%. = select i1 %cmp, i64 2, i64 1` , so each callsite is correctly instrumented (each index for one entry in the linked list>. Note simplify-cfg pass runs multiple times in a pipeline, configured with different options. SimplifyCFG pass before PGOInstrumentationGen doesn't sink or hoist as shown in the example.

For a SamplePGO + thinlto build,

1. if a function `foo` gets two indirect callsites `c1` and `c2` merged in the training build, hypothetically one debug location is kept in the merged callsite, or maybe both debug locations are dropped.
  - If both debug locations are dropped, the merged one callsite won't be annotated with value profiles.
  - If one debug location is kept,  target values of both `c1` and `c2` are attributed to this debug location, and indirect callsite for this debug location is annotated.
2. When collected sample-pgo profiles have two value profiles in the first place, sample-pgo-use is more likely to see two indirect callsites, each annotated with different value profiles.

> 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. That merged callsite will lead to a merged !VP profile. The already promoted callsites, if not merged, will lead to separate !VP profiles.

#2 discusses what happens when value profiles are merged on two different callsites.

For Sample-PGO, when two indirect callsites are merged in the first place in the training build (where sample-pgo profiles are collected), the value profile could be associated with at most one debug location, so at most one of the two indirect callsites could be annotated.

> 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 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.
>
> 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.

It's likely that indirect callsites cannot be merged because of the introduced if-else branches; there is a trade-off to be made here -> for the significant indirect calls that show up enough number of times (per `isPromotionProfitable`), speculatively devirtualize it (possibly inline it in the later inliner pass) is overall good.

If extending value profiles to make it one-bit more stateful is ok (in terms of code quality, profile size, etc), it preserves the original per-callsite distribution compared with merging two different callsites.

> Thoughts?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148876