[IndirectCallPromotion][SimpifyCFG] Preserve indirect callsites in hoist/sink when merging !prof is unprofitable and merge these callsites after ICP completes in a build pipeline

[mingmingl]

Summary

The problem to solve:

• When simplify-cfg could hoist / sink indirect callsites with inherently different target values, there isn't a simple lossless way to merge !prof value profile metadata.
  • hoist of indirect call https://gcc.godbolt.org/z/o6G68rn3v, sink of indirect call https://gcc.godbolt.org/z/79E3onono. The C++ comments and test cases have brief comments to explain why/when not preserving is sub-optimal. Basically, ICP heuristics (code) promotes when a target value show up enough times, and merging the sum changes the distribution.

This patch:

1. Before indirect-call-promotion happens, preserve the indirect callsites with target value profiles in simplify-cfg when there isn't a good way to merge.
   • Basically, when two indirect callsites each has value profiles, preserve them; or else it's fine to hoist or sink before ICP.
2. After indirect-call-promotion happens, always sink or hoist (when possible) and merge value profiles.

Implementation:

1. Set a module flag after the last ICP pass completes in a binary build pipeline.
   • Pass in ThinOrFullLTOPhase to ICP pass constructor. IndirectCallProm transformations all happen after ICP pass completes in a {thin, regular} LTO postlink pipeline or in a non-LTO pipeline.
2. Add helper functions to decide whether it's profitable to merge and get merged results.
3. SimplifyCFG calls helper functions above to preserve callsites for indirect-call-prom to happen, or hoist/sink indirect calls && merge value profiles.
4. Clearing value profiles after ICP makes use of them won't work since CGProfile pass still uses <target-value, counter> pairs to compute function hotness.

[tejohnson]

Would be good to explain motivation (maybe reference other patch?)

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

llvm/include/llvm/IR/Instructions.h

4018

Nothing in this patch uses the IndirectCallMD value set here - is it needed?

llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

309

This is unnecessary if !MDProf

313

Can the CallBase be anything other than a CallInst or InvokeInst?

408

Comment describing why

[wenlei]

cc @hoy to make sure this works for csspgo.

[mingmingl]

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

[hoy]

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

[mingmingl]

In D148876#4288686, @hoy wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

oh thanks for this pointer! Clearing is not an option then.

CGProfile.cpp uses <counter, target-val> pairs but not 'total-count', I'm suggesting this option. Let me know your thoughts!

In this patch

1. extend its format from !1 = !{!"VP", IndirectCallKind , Sum, counter1, val1, counter2, val2} to !1 = !{!"VP", Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'}
2. indirect-call-promotion-all-done is initially 0, and set to 1 after the last ICP in a compiler pipeline

In D147954

3. simplify-cfg bails out from merge indirect-calls when seeing indirect-call-promotion-all-done as 0, could merge target value profiles after seeing indirect-call-promotion-all-done being set to 1, so that CGProfile.cpp could get the <counter, target-val> pairs.

Meanwhile, the current ICP should probably still preserve counts as opposed to clearing when all targets are used.

cc @tejohnson @davidxl

[hoy]

In D148876#4288764, @mingmingl wrote:

In D148876#4288686, @hoy wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

oh thanks for this pointer! Clearing is not an option then.

CGProfile.cpp uses <counter, target-val> pairs but not 'total-count', I'm suggesting this option. Let me know your thoughts!

In this patch

1. extend its format from !1 = !{!"VP", IndirectCallKind , Sum, counter1, val1, counter2, val2} to !1 = !{!"VP", Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'}
2. indirect-call-promotion-all-done is initially 0, and set to 1 after the last ICP in a compiler pipeline

In D147954

3. simplify-cfg bails out from merge indirect-calls when seeing indirect-call-promotion-all-done as 0, could merge target value profiles after seeing indirect-call-promotion-all-done being set to 1, so that CGProfile.cpp could get the <counter, target-val> pairs.

Meanwhile, the current ICP should probably still preserve counts as opposed to clearing when all targets are used.

cc @tejohnson @davidxl

I guess I don't get the motivation of the original change. Can you please remind me why you'd like to cleanup VP metadata after ICP?

[mingmingl]

In D148876#4288774, @hoy wrote:

In D148876#4288764, @mingmingl wrote:

In D148876#4288686, @hoy wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

oh thanks for this pointer! Clearing is not an option then.

CGProfile.cpp uses <counter, target-val> pairs but not 'total-count', I'm suggesting this option. Let me know your thoughts!

In this patch

1. extend its format from !1 = !{!"VP", IndirectCallKind , Sum, counter1, val1, counter2, val2} to !1 = !{!"VP", Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'}
2. indirect-call-promotion-all-done is initially 0, and set to 1 after the last ICP in a compiler pipeline

In D147954

3. simplify-cfg bails out from merge indirect-calls when seeing indirect-call-promotion-all-done as 0, could merge target value profiles after seeing indirect-call-promotion-all-done being set to 1, so that CGProfile.cpp could get the <counter, target-val> pairs.

Meanwhile, the current ICP should probably still preserve counts as opposed to clearing when all targets are used.

cc @tejohnson @davidxl

I guess I don't get the motivation of the original change. Can you please remind me why you'd like to cleanup VP metadata after ICP?

(D147954 is about the problem to solve and has more discussions) In brief, https://gcc.godbolt.org/z/Pxoh8PrGE shows a case where simplify-cfg aggressively drops profile metadata while it shouldn't. This would happen when prelink simplify-cfg enables instruction sink and postlink ICP couldn't see the value profiles.
The motivation of this patch and D147954 is to preserve value profiles attached on indirect calls for indirect-call-promotion to use.

Turning off simplify-cfg sinking/hoisting completely in prelink pipeline would be more invasive -> for 'llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll' in D147954, it's desired to hoist address calculation instructions (from if.then and if.else in https://gcc.godbolt.org/z/b4Y9Mqrq1) in prelink stage.

[hoy]

In D148876#4288824, @mingmingl wrote:

In D148876#4288774, @hoy wrote:

In D148876#4288764, @mingmingl wrote:

In D148876#4288686, @hoy wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

oh thanks for this pointer! Clearing is not an option then.

CGProfile.cpp uses <counter, target-val> pairs but not 'total-count', I'm suggesting this option. Let me know your thoughts!

In this patch

1. extend its format from !1 = !{!"VP", IndirectCallKind , Sum, counter1, val1, counter2, val2} to !1 = !{!"VP", Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'}
2. indirect-call-promotion-all-done is initially 0, and set to 1 after the last ICP in a compiler pipeline

In D147954

3. simplify-cfg bails out from merge indirect-calls when seeing indirect-call-promotion-all-done as 0, could merge target value profiles after seeing indirect-call-promotion-all-done being set to 1, so that CGProfile.cpp could get the <counter, target-val> pairs.

Meanwhile, the current ICP should probably still preserve counts as opposed to clearing when all targets are used.

cc @tejohnson @davidxl

I guess I don't get the motivation of the original change. Can you please remind me why you'd like to cleanup VP metadata after ICP?

(D147954 is about the problem to solve and has more discussions) In brief, https://gcc.godbolt.org/z/Pxoh8PrGE shows a case where simplify-cfg aggressively drops profile metadata while it shouldn't. This would happen when prelink simplify-cfg enables instruction sink and postlink ICP couldn't see the value profiles.
The motivation of this patch and D147954 is to preserve value profiles attached on indirect calls for indirect-call-promotion to use.

Turning off simplify-cfg sinking/hoisting completely in prelink pipeline would be more invasive -> for 'llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll' in D147954, it's desired to hoist address calculation instructions (from if.then and if.else in https://gcc.godbolt.org/z/b4Y9Mqrq1) in prelink stage.

Thanks for the explanation. IIUC, the fix for SimplyCFG dropping !VP is to either block the corresponding optimization or to merge the !VP data when merge callsites. What is the problem of always merging callsite !VP?

[mingmingl]

In D148876#4288873, @hoy wrote:

In D148876#4288824, @mingmingl wrote:

In D148876#4288774, @hoy wrote:

In D148876#4288764, @mingmingl wrote:

In D148876#4288686, @hoy wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Also, the new tests only test the behavior with the internal option. It would be good to have tests with various pipelines to check the logic on when it gets cleared in those different cases simply based on the new pipeline logic.

I see your point. I'm going to write a test like https://github.com/llvm/llvm-project/blob/main/llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll to test pipeline logic.

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

thanks! The assumption here is that ICP runs once per pipeline and there are no other passes that uses value-profiles. If the assumption doesn't hold, I'm thinking about an explicit boolean in ICP pass constructor (which makes pipeline logic a little bit more complex)

Value profiles for indirect call sites can still be used by lld call graph sorting after ICP. Please see https://github.com/llvm/llvm-project/blob/26202a57e5e78b5473ef657737d181f0a68ed56d/llvm/lib/Transforms/Instrumentation/CGProfile.cpp#L81

oh thanks for this pointer! Clearing is not an option then.

CGProfile.cpp uses <counter, target-val> pairs but not 'total-count', I'm suggesting this option. Let me know your thoughts!

In this patch

1. extend its format from !1 = !{!"VP", IndirectCallKind , Sum, counter1, val1, counter2, val2} to !1 = !{!"VP", Kind , Sum, counter1, val1, counter2, val2, boolean/int 'indirect-call-promotion-all-done'}
2. indirect-call-promotion-all-done is initially 0, and set to 1 after the last ICP in a compiler pipeline

In D147954

3. simplify-cfg bails out from merge indirect-calls when seeing indirect-call-promotion-all-done as 0, could merge target value profiles after seeing indirect-call-promotion-all-done being set to 1, so that CGProfile.cpp could get the <counter, target-val> pairs.

Meanwhile, the current ICP should probably still preserve counts as opposed to clearing when all targets are used.

cc @tejohnson @davidxl

I guess I don't get the motivation of the original change. Can you please remind me why you'd like to cleanup VP metadata after ICP?

(D147954 is about the problem to solve and has more discussions) In brief, https://gcc.godbolt.org/z/Pxoh8PrGE shows a case where simplify-cfg aggressively drops profile metadata while it shouldn't. This would happen when prelink simplify-cfg enables instruction sink and postlink ICP couldn't see the value profiles.
The motivation of this patch and D147954 is to preserve value profiles attached on indirect calls for indirect-call-promotion to use.

Turning off simplify-cfg sinking/hoisting completely in prelink pipeline would be more invasive -> for 'llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll' in D147954, it's desired to hoist address calculation instructions (from if.then and if.else in https://gcc.godbolt.org/z/b4Y9Mqrq1) in prelink stage.

Thanks for the explanation. IIUC, the fix for SimplyCFG dropping !VP is to either block the corresponding optimization or to merge the !VP data when merge callsites. What is the problem of always merging callsite !VP?

Take https://gcc.godbolt.org/z/o6G68rn3v as example, func1 and func2 are two different virtual methods so inherently has different target values.

Say each virtual callsite has a sum and a list of <counter, target-value> pairs, merging <counter, target-value> pairs is okay, either merging sum, or use max(sum1, sum2) changed the ratio, and could make a significant target value no longer profitable according to isPromotionProfitable

[wenlei]

Thanks for the explanation. IIUC, the fix for SimplyCFG dropping !VP is to either block the corresponding optimization or to merge the !VP data when merge callsites. What is the problem of always merging callsite !VP?

@hoy the problem is you lose flow-sensitivity for the VP after merge. And since ICP is important, it perhaps warrant special handling.

[hoy]

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

Thoughts?

[davidxl]

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

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

}

// After merging in profile binary we get

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

// After sampling, line 3 gets VP data 'bar', 'blah'

// In optimized build, assuming merge does not happen before sample loading, the following will happen:

 

-   fp1 gets VP data 'bar', and 'blah' and get promoted, 
-   fp2 at line 5 has no VP data
-   merging will be prevented
-   line 3 will be promoted (less precisedly)
-   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?

[davidxl]

Note the format of my last reply is messed up a little.

[mingmingl]

In 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 (and already made when indirect calls are promoted today) -> 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.

[hoy]

Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:

1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.

2. For callsites originally from same location but ended up with different inline contexts in the caller function, the merger tries to reconcile their inline contexts by finding a common suffix of the contexts. If successful, the common suffix will be the debug location for the merged callsites. This may allow for the value profile to be shared by the two callsites (under a different inlining situation). E.g, for a callsite defined in function D at line 4, if somehow it gets inlined twice into function A:

A:1 @ B:2 @ C:3 @ D:4

A:2 @ B:3 @ C:3 @ D:4

Then my understanding is that the merged callsite will have C:3 @ D:4 as its debug string. If somehow in the next compilation A never inlines B but B inlines C twice, then the two callsites could end up sharing the same value profile.

#2 may sound uncommon, but #1 should be common. If that's the case, then we basically lose track of value profiles for merged callsites.

Please let me know if I miss anything. Despite AutoFDO, I think Mingming's solution that allows merging only after ICP should be good for Instrument PGO.

[davidxl]

In D148876#4289326, @hoy wrote:

Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:

1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.

Thanks for checking. I suspect bugs like this may exist, but it is orthogonal to the problem at hand.

2. For callsites originally from same location but ended up with different inline contexts in the caller function, the merger tries to reconcile their inline contexts by finding a common suffix of the contexts. If successful, the common suffix will be the debug location for the merged callsites. This may allow for the value profile to be shared by the two callsites (under a different inlining situation). E.g, for a callsite defined in function D at line 4, if somehow it gets inlined twice into function A:

A:1 @ B:2 @ C:3 @ D:4

A:2 @ B:3 @ C:3 @ D:4

Then my understanding is that the merged callsite will have C:3 @ D:4 as its debug string. If somehow in the next compilation A never inlines B but B inlines C twice, then the two callsites could end up sharing the same value profile.

True, but this happens today as well -- it is very unlikely such two callsites from inline instances can be merged in the first place.

#2 may sound uncommon,

yes. Besides sharing profile may still be better than dropping profile.

> but #1 should be common. If that's the case, then we basically lose track of value profiles for merged callsites.

Please let me know if I miss anything.

See my reply above.

Despite AutoFDO, I think Mingming's solution that allows merging only after ICP should be good for Instrument PGO.

[xur]

In D148876#4289423, @davidxl wrote:

In D148876#4289326, @hoy wrote:

Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:

1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.

Thanks for checking. I suspect bugs like this may exist, but it is orthogonal to the problem at hand.

I don't think this is a bug. This is the intended behavior -- if there is conflict debug info, just set to zero line number of remove the debug info.
This does create some problems for Sample FDO.

This is not a problem for pseudo-probe, right?

2. For callsites originally from same location but ended up with different inline contexts in the caller function, the merger tries to reconcile their inline contexts by finding a common suffix of the contexts. If successful, the common suffix will be the debug location for the merged callsites. This may allow for the value profile to be shared by the two callsites (under a different inlining situation). E.g, for a callsite defined in function D at line 4, if somehow it gets inlined twice into function A:

A:1 @ B:2 @ C:3 @ D:4

A:2 @ B:3 @ C:3 @ D:4

Then my understanding is that the merged callsite will have C:3 @ D:4 as its debug string. If somehow in the next compilation A never inlines B but B inlines C twice, then the two callsites could end up sharing the same value profile.

True, but this happens today as well -- it is very unlikely such two callsites from inline instances can be merged in the first place.

#2 may sound uncommon,

yes. Besides sharing profile may still be better than dropping profile.

> but #1 should be common. If that's the case, then we basically lose track of value profiles for merged callsites.

Please let me know if I miss anything.

See my reply above.

Despite AutoFDO, I think Mingming's solution that allows merging only after ICP should be good for Instrument PGO.

This does not make AutoFDO worse, right?
This seems also to benefit pseudo-probe based AutoFDO.

[hoy]

In D148876#4293319, @xur wrote:

In D148876#4289423, @davidxl wrote:

In D148876#4289326, @hoy wrote:

Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:

1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.

Thanks for checking. I suspect bugs like this may exist, but it is orthogonal to the problem at hand.

I don't think this is a bug. This is the intended behavior -- if there is conflict debug info, just set to zero line number of remove the debug info.
This does create some problems for Sample FDO.

This is not a problem for pseudo-probe, right?

Unfortunately it somehow still affects pseudo probe which uses !dbg info for callsites to store their probe information.

2. For callsites originally from same location but ended up with different inline contexts in the caller function, the merger tries to reconcile their inline contexts by finding a common suffix of the contexts. If successful, the common suffix will be the debug location for the merged callsites. This may allow for the value profile to be shared by the two callsites (under a different inlining situation). E.g, for a callsite defined in function D at line 4, if somehow it gets inlined twice into function A:

A:1 @ B:2 @ C:3 @ D:4

A:2 @ B:3 @ C:3 @ D:4

Then my understanding is that the merged callsite will have C:3 @ D:4 as its debug string. If somehow in the next compilation A never inlines B but B inlines C twice, then the two callsites could end up sharing the same value profile.

True, but this happens today as well -- it is very unlikely such two callsites from inline instances can be merged in the first place.

#2 may sound uncommon,

yes. Besides sharing profile may still be better than dropping profile.

> but #1 should be common. If that's the case, then we basically lose track of value profiles for merged callsites.

Please let me know if I miss anything.

See my reply above.

Despite AutoFDO, I think Mingming's solution that allows merging only after ICP should be good for Instrument PGO.

This does not make AutoFDO worse, right?
This seems also to benefit pseudo-probe based AutoFDO.

Right, it doesn't make AutoFDO worse. It should help pseudo probe in some cases where merging biases one callsite.

[wenlei]

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

Sorry for being vague. What I meant is to double check the pass ordering to make sure csspgo doesn't have extra use of VP after sample loader and ICP, so clearing is ok.

CGProfile is a good catch. Now if the final solution is to allow merging of VP after ICP is done, instead clearing all VP, I don't see any problem then.

[wenlei]

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Can I piggyback on this to suggest a general feedback :) Reviewers would benefit from more explanation on why, in addition to what, and it applies to other patches as well. Without asking explicitly, I failed to grasp your intention of D147954 just by looking at the patch without asking questions. It somewhat applies to the profile reader changes too.

[mingmingl]

In D148876#4297923, @wenlei wrote:

In D148876#4288429, @wenlei wrote:

cc @hoy to make sure this works for csspgo.

Sorry for being vague. What I meant is to double check the pass ordering to make sure csspgo doesn't have extra use of VP after sample loader and ICP, so clearing is ok.

CGProfile is a good catch. Now if the final solution is to allow merging of VP after ICP is done, instead clearing all VP, I don't see any problem then.

Yep without the catch the clearing option would be bad. So indeed good catch!

As an update, I'll proceed to implement the idea of allow merging after ICP transformation, with slightly better implementations than extending "vp" format;

1. given allow-merging is either true or false for all value-profile instances depending on the execution stage of pipeline, use per module flag rather than per !prof boolean
2. If possible, introduce helper functions for individual transformation passes (simplifycfg or cse or whatever pass) to call, the helper function either returns 'wait, don't merge for now' (maybe fleshes out as nullptr) or the result of the merged instructions (with merged value profiles).

This way, 1) reduces repeated states to a per-module level, and 2) makes it easier to preserve metadata for other passes (not only simplify-cfg, might be early-cse pass, etc).

In D148876#4297926, @wenlei wrote:

In D148876#4288504, @mingmingl wrote:

In D148876#4288286, @tejohnson wrote:

Would be good to explain motivation (maybe reference other patch?)

Makes sense, will do.

Can I piggyback on this to suggest a general feedback :) Reviewers would benefit from more explanation on why, in addition to what, and it applies to other patches as well. Without asking explicitly, I failed to grasp your intention of D147954 just by looking at the patch without asking questions. It somewhat applies to the profile reader changes too.

Sure, I'll try to do better. My take is that natural language could be ambiguous compared with reduced godbolt examples (or diagrams to describe cfg, etc). Even with how-and-why explained, I could imagine questions are welcome rather than being frowned upon, like how existing questions inspires me to learn and find problems early.

[mingmingl]

resolve comments and absorbs D147954 to illustrate how module flag and helper functions are used.. Could split out to smaller patches as well.

• described the problem to solve in summary.
• modified existing tests for pass pipeline changes.
• (implementation) consider the possibility that an invoke-instruction could have both branch_weights'and value profiles.

[mingmingl]

llvm/include/llvm/IR/Instructions.h

4018

In the previous version this function was not called so should be deleted back then. Only getBranchWeightProfData was used. Now the change becomes obsolete..

llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

309

Done in the sense that the code is obsolete

313

Good question. Only CallInst or InvokeInst should be relevant for function calls, since CallBrInst is [[ I see many isa<CallInst>(I) || isa<Invokeinst> in the codebase. | only used for inline assembly goto ]].

Added comment around helper function getFunctionCallInstr in llvm/lib/IR/Metadata.cpp for this.

llvm/include/llvm/IR/Instructions.h

public:
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Instruction *I) {
     return (I->getOpcode() == Instruction::Invoke);
   }
   static bool classof(const Value *V) {
     return isa<Instruction>(V) && classof(cast<Instruction>(V));
   }
 
+  static bool getProfMetaData(MDNode *MD, MDNode *&BranchWeightMD,
+                              MDNode *&IndirectCallMD) {
+    BranchWeightMD = IndirectCallMD = nullptr;
+
+    auto isMDwithName = [](MDNode *MD, const char *Str) {
+      auto *MDName = dyn_cast<MDString>(MD->getOperand(0));
+      if (!MDName)
+        return false;
+      return (MDName->getString().equals(Str));
+    };
+
+    if (isMDwithName(MD, "branch_weights")) {
+      BranchWeightMD = MD;
+      return true;
+    }
+    if (isMDwithName(MD, "VP")) {
+      IndirectCallMD = MD;
+      return true;
+    }
+    if (MD->getNumOperands() != 3)
+      return false;
+    for (int I = 1; I < 3; I++) {
+      auto *MDT = cast<MDNode>(MD->getOperand(I));
+      if (!MDT)
+        continue;
+      if (isMDwithName(MDT, "branch_weights"))
+        BranchWeightMD = MDT;
+      else if (isMDwithName(MDT, "VP"))
+        IndirectCallMD = MDT;
+    }
+    return (BranchWeightMD || IndirectCallMD);
+  }
+
+  // A utility method that return "branch_weights" metadata. The return
+  // value will be nullptr if the metadata is not found.
+  static MDNode *getBranchWeightProfData(MDNode *MD) {
+    MDNode *BranchWeightMD, *IndirectCallMD;
+    getProfMetaData(MD, BranchWeightMD, IndirectCallMD);
+    return BranchWeightMD;
+  }
+
 private:
   // Shadow Instruction::setInstructionSubclassData with a private forwarding
   // method so that subclasses cannot accidentally use it.
   template <typename Bitfield>
   void setSubclassData(typename Bitfield::Type Value) {
     Instruction::setSubclassData<Bitfield>(Value);
   }
 };

llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_INSTRUMENTATION_PGOINSTRUMENTATION_H
 #define LLVM_TRANSFORMS_INSTRUMENTATION_PGOINSTRUMENTATION_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Pass.h"
 #include <cstdint>
 #include <string>
 
 namespace llvm {
 
 class Function;
 class Instruction;
 class Module;
private:
   // If this is a context sensitive instrumentation.
   bool IsCS;
   IntrusiveRefCntPtr<vfs::FileSystem> FS;
 };
 
 /// The indirect function call promotion pass.
 class PGOIndirectCallPromotion : public PassInfoMixin<PGOIndirectCallPromotion> {
 public:
-  PGOIndirectCallPromotion(bool IsInLTO = false, bool SamplePGO = false)
-      : InLTO(IsInLTO), SamplePGO(SamplePGO) {}
+  PGOIndirectCallPromotion(
+      ThinOrFullLTOPhase Phase = ThinOrFullLTOPhase::ThinLTOPreLink,
+      bool SamplePGO = false)
+      : Phase(Phase), SamplePGO(SamplePGO) {}
 
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
 
 private:
-  bool InLTO;
+  ThinOrFullLTOPhase Phase;
   bool SamplePGO;
 };
 
 /// The profile size based optimization pass for memory intrinsics.
 class PGOMemOPSizeOpt : public PassInfoMixin<PGOMemOPSizeOpt> {
 public:
   PGOMemOPSizeOpt() = default;
 

llvm/lib/Passes/PassBuilderPipelines.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   // passed to the compile) to the SamplePGO flag of ICP. This is used to
   // determine whether the new direct calls are annotated with prof metadata.
   // Ideally this should be determined from whether the IR is annotated with
   // sample profile, and not whether the a sample profile was provided on the
   // command line. E.g. for flattened profiles where we will not be reloading
   // the sample profile in the ThinLTO backend, we ideally shouldn't have to
   // provide the sample profile file.
   if (Phase == ThinOrFullLTOPhase::ThinLTOPostLink && !LoadSampleProfile)
-    MPM.addPass(PGOIndirectCallPromotion(true /* InLTO */, HasSampleProfile));
+    MPM.addPass(PGOIndirectCallPromotion(Phase, HasSampleProfile));
 
   // Do basic inference of function attributes from known properties of system
   // libraries and other oracles.
   MPM.addPass(InferFunctionAttrsPass());
   MPM.addPass(CoroEarlyPass());
 
   // Create an early function pass manager to cleanup the output of the
   // frontend.
if (LoadSampleProfile) {
     MPM.addPass(RequireAnalysisPass<ProfileSummaryAnalysis, Module>());
     // Do not invoke ICP in the LTOPrelink phase as it makes it hard
     // for the profile annotation to be accurate in the LTO backend.
     if (!isLTOPreLink(Phase))
       // We perform early indirect call promotion here, before globalopt.
       // This is important for the ThinLTO backend phase because otherwise
       // imported available_externally functions look unreferenced and are
       // removed.
-      MPM.addPass(
-          PGOIndirectCallPromotion(true /* IsInLTO */, true /* SamplePGO */));
+      MPM.addPass(PGOIndirectCallPromotion(Phase, true /* SamplePGO */));
   }
 
   // Try to perform OpenMP specific optimizations on the module. This is a
   // (quick!) no-op if there are no OpenMP runtime calls present in the module.
   MPM.addPass(OpenMPOptPass());
 
   if (AttributorRun & AttributorRunOption::MODULE)
     MPM.addPass(AttributorPass());
PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   // Add all the requested passes for instrumentation PGO, if requested.
   if (PGOOpt && Phase != ThinOrFullLTOPhase::ThinLTOPostLink &&
       (PGOOpt->Action == PGOOptions::IRInstr ||
        PGOOpt->Action == PGOOptions::IRUse)) {
     addPGOInstrPasses(MPM, Level,
                       /* RunProfileGen */ PGOOpt->Action == PGOOptions::IRInstr,
                       /* IsCS */ false, PGOOpt->ProfileFile,
                       PGOOpt->ProfileRemappingFile, Phase, PGOOpt->FS);
-    MPM.addPass(PGOIndirectCallPromotion(false, false));
+    MPM.addPass(PGOIndirectCallPromotion(Phase, false /* SamplePGO */));
   }
   if (PGOOpt && Phase != ThinOrFullLTOPhase::ThinLTOPostLink &&
       PGOOpt->CSAction == PGOOptions::CSIRInstr)
     MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->CSProfileGenFile));
 
   // Synthesize function entry counts for non-PGO compilation.
   if (EnableSyntheticCounts && !PGOOpt)
     MPM.addPass(SyntheticCountsPropagation());
PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
   if (Level.getSpeedupLevel() > 1) {
     MPM.addPass(createModuleToFunctionPassAdaptor(
         CallSiteSplittingPass(), PTO.EagerlyInvalidateAnalyses));
 
     // Indirect call promotion. This should promote all the targets that are
     // left by the earlier promotion pass that promotes intra-module targets.
     // This two-step promotion is to save the compile time. For LTO, it should
     // produce the same result as if we only do promotion here.
-    MPM.addPass(PGOIndirectCallPromotion(
-        true /* InLTO */, PGOOpt && PGOOpt->Action == PGOOptions::SampleUse));
+    MPM.addPass(PGOIndirectCallPromotion(ThinOrFullLTOPhase::FullLTOPostLink,
+                                         PGOOpt && PGOOpt->Action ==
+                                                       PGOOptions::SampleUse));
 
     // Propagate constants at call sites into the functions they call.  This
     // opens opportunities for globalopt (and inlining) by substituting function
     // pointers passed as arguments to direct uses of functions.
     MPM.addPass(IPSCCPPass(IPSCCPOptions(/*AllowFuncSpec=*/
                                          Level != OptimizationLevel::Os &&
                                          Level != OptimizationLevel::Oz)));
 

llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

static cl::opt<bool> ICPInvokeOnly("icp-invoke-only", cl::init(false),
                                             "invoke instruction only"));
 
 // Dump the function level IR if the transformation happened in this
 // function. For debug use only.
 static cl::opt<bool>
     ICPDUMPAFTER("icp-dumpafter", cl::init(false), cl::Hidden,
                  cl::desc("Dump IR after transformation happens"));
 
+static cl::opt<bool> ICPClearTargetValueProfiles(
+    "icp-clear-target-value-profiles", cl::init(false), cl::Hidden,
+    cl::desc("If set, override the "
+             "PGOIndirectCallPromotion::ClearValueProfileMetadata. For testing "
+             "and debugging purpose only"));
+
 namespace {
 
 // The class for main data structure to promote indirect calls to conditional
 // direct calls.
 class ICallPromotionFunc {
 private:
   Function &F;
   Module *M;
 
   // Symtab that maps indirect call profile values to function names and
   // defines.
   InstrProfSymtab *Symtab;
 
   bool SamplePGO;
+  bool ClearValueProfileMetadata = false;
 
   OptimizationRemarkEmitter &ORE;
 
   // A struct that records the direct target and it's call count.
   struct PromotionCandidate {
     Function *TargetFunction;
     uint64_t Count;
 
private:
   // Promote a list of targets for one indirect-call callsite. Return
   // the number of promotions.
   uint32_t tryToPromote(CallBase &CB,
                         const std::vector<PromotionCandidate> &Candidates,
                         uint64_t &TotalCount);
 
 public:
   ICallPromotionFunc(Function &Func, Module *Modu, InstrProfSymtab *Symtab,
-                     bool SamplePGO, OptimizationRemarkEmitter &ORE)
-      : F(Func), M(Modu), Symtab(Symtab), SamplePGO(SamplePGO), ORE(ORE) {}
+                     bool SamplePGO, bool ClearValueProfileMetadata,
+                     OptimizationRemarkEmitter &ORE)
+      : F(Func), M(Modu), Symtab(Symtab), SamplePGO(SamplePGO),
+        ClearValueProfileMetadata(ClearValueProfileMetadata), ORE(ORE) {}
   ICallPromotionFunc(const ICallPromotionFunc &) = delete;
   ICallPromotionFunc &operator=(const ICallPromotionFunc &) = delete;
 
   bool processFunction(ProfileSummaryInfo *PSI);
 };
 
 } // end anonymous namespace
 
for (const auto &C : Candidates) {
     assert(TotalCount >= Count);
     TotalCount -= Count;
     NumOfPGOICallPromotion++;
     NumPromoted++;
   }
   return NumPromoted;
 }
 
+static void clearValueProfile(CallBase *CB) {
+  assert(CB != nullptr && "Caller guaranteed");
+  MDNode *MDProf = CB->getMetadata(LLVMContext::MD_prof);
+  if (!MDProf || isa<CallInst>(CB)) {
+    CB->setMetadata(LLVMContext::MD_prof, nullptr);
+    return;
+  }
+
+  if (!isa<InvokeInst>(CB))
+    return;
+
+  CB->setMetadata(LLVMContext::MD_prof,
+                  InvokeInst::getBranchWeightProfData(MDProf));
+}
+
 // Traverse all the indirect-call callsite and get the value profile
 // annotation to perform indirect-call promotion.
 bool ICallPromotionFunc::processFunction(ProfileSummaryInfo *PSI) {
   bool Changed = false;
   ICallPromotionAnalysis ICallAnalysis;
   for (auto *CB : findIndirectCalls(F)) {
     uint32_t NumVals, NumCandidates;
     uint64_t TotalCount;
     auto ICallProfDataRef = ICallAnalysis.getPromotionCandidatesForInstruction(
         CB, NumVals, TotalCount, NumCandidates);
     if (!NumCandidates ||
         (PSI && PSI->hasProfileSummary() && !PSI->isHotCount(TotalCount)))
       continue;
     auto PromotionCandidates = getPromotionCandidatesForCallSite(
         *CB, ICallProfDataRef, TotalCount, NumCandidates);
     uint32_t NumPromoted = tryToPromote(*CB, PromotionCandidates, TotalCount);
+
+    if (ClearValueProfileMetadata) {
+      clearValueProfile(CB);
+      continue;
+    }
     if (NumPromoted == 0)
       continue;
 
     Changed = true;
     // Adjust the MD.prof metadata. First delete the old one.
     CB->setMetadata(LLVMContext::MD_prof, nullptr);
     // If all promoted, we don't need the MD.prof metadata.
     if (TotalCount == 0 || NumPromoted == NumVals)
       continue;
     // Otherwise we need update with the un-promoted records back.
     annotateValueSite(*M, *CB, ICallProfDataRef.slice(NumPromoted), TotalCount,
                       IPVK_IndirectCallTarget, NumCandidates);
   }
   return Changed;
 }
 
 // A wrapper function that does the actual work.
-static bool promoteIndirectCalls(Module &M, ProfileSummaryInfo *PSI,
-                                 bool InLTO, bool SamplePGO,
+static bool promoteIndirectCalls(Module &M, ProfileSummaryInfo *PSI, bool InLTO,
+                                 bool SamplePGO, bool ClearValueProfileMetadata,
                                  ModuleAnalysisManager *AM = nullptr) {
   if (DisableICP)
     return false;
   InstrProfSymtab Symtab;
   if (Error E = Symtab.create(M, InLTO)) {
     std::string SymtabFailure = toString(std::move(E));
     M.getContext().emitError("Failed to create symtab: " + SymtabFailure);
     return false;
if (AM) {
       auto &FAM =
           AM->getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
       ORE = &FAM.getResult<OptimizationRemarkEmitterAnalysis>(F);
     } else {
       OwnedORE = std::make_unique<OptimizationRemarkEmitter>(&F);
       ORE = OwnedORE.get();
     }
 
-    ICallPromotionFunc ICallPromotion(F, &M, &Symtab, SamplePGO, *ORE);
+    ICallPromotionFunc ICallPromotion(F, &M, &Symtab, SamplePGO,
+                                      ClearValueProfileMetadata, *ORE);
     bool FuncChanged = ICallPromotion.processFunction(PSI);
     if (ICPDUMPAFTER && FuncChanged) {
       LLVM_DEBUG(dbgs() << "\n== IR Dump After =="; F.print(dbgs()));
       LLVM_DEBUG(dbgs() << "\n");
     }
     Changed |= FuncChanged;
     if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {
       LLVM_DEBUG(dbgs() << " Stop: Cutoff reached.\n");
       break;
     }
   }
   return Changed;
 }
 
 PreservedAnalyses PGOIndirectCallPromotion::run(Module &M,
                                                 ModuleAnalysisManager &AM) {
   ProfileSummaryInfo *PSI = &AM.getResult<ProfileSummaryAnalysis>(M);
 
+  const bool InLTO = (Phase == ThinOrFullLTOPhase::ThinLTOPostLink ||
+                      Phase == ThinOrFullLTOPhase::FullLTOPostLink);
+
+  bool ClearValueProfileMetadata =
+      (Phase != ThinOrFullLTOPhase::ThinLTOPreLink &&
+       Phase != ThinOrFullLTOPhase::FullLTOPreLink);
+  if (ICPClearTargetValueProfiles.getNumOccurrences())
+    ClearValueProfileMetadata = ICPClearTargetValueProfiles;
+
   if (!promoteIndirectCalls(M, PSI, InLTO | ICPLTOMode,
-                            SamplePGO | ICPSamplePGOMode, &AM))
+                            SamplePGO | ICPSamplePGOMode,
+                            ClearValueProfileMetadata, &AM))
     return PreservedAnalyses::all();
 
   return PreservedAnalyses::none();
 }

llvm/test/Transforms/PGOProfile/indirect_call_promotion.ll

 ; RUN: opt < %s -passes=pgo-icall-prom -S -icp-total-percent-threshold=50 | FileCheck %s --check-prefix=ICALL-PROM
+; RUN: opt < %s -passes=pgo-icall-prom -S -icp-total-percent-threshold=50 -icp-clear-target-value-profiles | FileCheck %s --check-prefix=ICALL-PROM-NO-PROF
 ; RUN: opt < %s -passes=pgo-icall-prom -S -pass-remarks=pgo-icall-prom -icp-remaining-percent-threshold=0 -icp-total-percent-threshold=0 -icp-max-prom=4 2>&1 | FileCheck %s --check-prefix=PASS-REMARK
 ; RUN: opt < %s -passes=pgo-icall-prom -S -pass-remarks=pgo-icall-prom -icp-remaining-percent-threshold=0 -icp-total-percent-threshold=20 -icp-max-prom=4 2>&1 | FileCheck %s --check-prefix=PASS2-REMARK
 
 ; PASS-REMARK: remark: <unknown>:0:0: Promote indirect call to func4 with count 1030 out of 1600
 ; PASS-REMARK: remark: <unknown>:0:0: Promote indirect call to func2 with count 410 out of 570
 ; PASS-REMARK: remark: <unknown>:0:0: Promote indirect call to func3 with count 150 out of 160
 ; PASS-REMARK: remark: <unknown>:0:0: Promote indirect call to func1 with count 10 out of 10
 

 ; ICALL-PROM:   br i1 [[CMP]], label %if.true.direct_targ, label %if.false.orig_indirect, !prof [[BRANCH_WEIGHT:![0-9]+]]
 ; ICALL-PROM: if.true.direct_targ:
 ; ICALL-PROM:   [[DIRCALL_RET:%[0-9]+]] = call i32 @func4()
 ; ICALL-PROM-SAMPLEPGO: call i32 @func4(), !prof [[CALL_METADATA:![0-9]+]]
 ; ICALL-PROM:   br label %if.end.icp
   %call = call i32 %tmp(), !prof !1
 ; ICALL-PROM: if.false.orig_indirect:
 ; ICALL-PROM:   %call = call i32 %tmp(), !prof [[NEW_VP_METADATA:![0-9]+]]
+; ICALL-PROM-NO-PROF:   %call = call i32 %tmp()
   ret i32 %call
 ; ICALL-PROM: if.end.icp:
 ; ICALL-PROM:   [[PHI_RET:%[0-9]+]] = phi i32 [ %call, %if.false.orig_indirect ], [ [[DIRCALL_RET]], %if.true.direct_targ ]
 ; ICALL-PROM:   ret i32 [[PHI_RET]]
 }
 
 !1 = !{!"VP", i32 0, i64 1600, i64 7651369219802541373, i64 1030, i64 -4377547752858689819, i64 410, i64 -6929281286627296573, i64 150, i64 -2545542355363006406, i64 10}
 
 ; ICALL-PROM: [[BRANCH_WEIGHT]] = !{!"branch_weights", i32 1030, i32 570}
 ; ICALL-PROM: [[NEW_VP_METADATA]] = !{!"VP", i32 0, i64 570, i64 -4377547752858689819, i64 410}
 ; ICALL-PROM-SAMPLEPGO: [[CALL_METADATA]] = !{!"branch_weights", i32 1030}

llvm/test/Transforms/PGOProfile/indirect_call_promotion_musttail_typecheck.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; Mustcall needs stricter parameter type checks otherwise it will fail in verifier.
 
-; RUN: opt < %s -passes=pgo-icall-prom -S | FileCheck %s
+; RUN: opt < %s -passes=pgo-icall-prom -S | FileCheck %s --check-prefixes=CHECK,CHECK_ICP_PROF
+; RUN: opt < %s -passes=pgo-icall-prom -icp-clear-target-value-profiles -S | FileCheck %s --check-prefixes=CHECK,CHECK_NO_ICP_PROF
 
 ; Here we check there is no ICP due to parameter mismatch.
 define ptr @func(ptr %msg, ptr %ptr, ptr %ctx, i64 %data.coerce, ptr %table, i64 %hasbits) {
 ; CHECK-LABEL: @func(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr null, align 8
 ; CHECK-NEXT:    ret ptr null
 ; CHECK:       1:
-; CHECK-NEXT:    [[CALL11_I:%.*]] = musttail call ptr [[TMP0]](ptr null, ptr null, ptr null, i64 0, ptr null, i64 0), !prof [[PROF0:![0-9]+]]
+; CHECK_ICP_PROF-NEXT:    [[CALL11_I:%.*]] = musttail call ptr [[TMP0]](ptr null, ptr null, ptr null, i64 0, ptr null, i64 0), !prof [[PROF0:![0-9]+]]
+; CHECK_NO_ICP_PROF-NEXT:    [[CALL11_I:%.*]] = musttail call ptr [[TMP0]](ptr null, ptr null, ptr null, i64 0, ptr null, i64 0)
 ; CHECK-NEXT:    ret ptr [[CALL11_I]]
 ;
 entry:
   %0 = load ptr, ptr null, align 8
   ret ptr null
 
 1:
   %call11.i = musttail call ptr %0(ptr null, ptr null, ptr null, i64 0, ptr null, i64 0), !prof !0
   ret ptr %call11.i
 }
 
 ; Here we check that ICP succeeds since parameters match. Also check the direct-call has a MustCall attribute.
 define ptr @func2(ptr %msg, i64 %tag, ptr %ctx, ptr %type, ptr %table, ptr %ptr) {
 ; CHECK-LABEL: @func2(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP1:%.*]] = load ptr, ptr null, align 8
 ; CHECK-NEXT:    ret ptr null
 ; CHECK:       1:
 ; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq ptr [[TMP1]], @_ZN6proto28internal12ExtensionSet10ParseFieldEmPKcPKNS_7MessageEPNS0_16InternalMetadataEPNS0_12ParseContextE
 ; CHECK-NEXT:    br i1 [[TMP2]], label [[IF_TRUE_DIRECT_TARG:%.*]], label [[TMP4:%.*]], !prof [[PROF1:![0-9]+]]
 ; CHECK:       if.true.direct_targ:
 ; CHECK-NEXT:    [[TMP3:%.*]] = musttail call ptr @_ZN6proto28internal12ExtensionSet10ParseFieldEmPKcPKNS_7MessageEPNS0_16InternalMetadataEPNS0_12ParseContextE(ptr null, i64 0, ptr null, ptr null, ptr null, ptr null)
 ; CHECK-NEXT:    ret ptr [[TMP3]]
 ; CHECK:       4:
-; CHECK-NEXT:    [[CALL11_I:%.*]] = musttail call ptr [[TMP1]](ptr null, i64 0, ptr null, ptr null, ptr null, ptr null), !prof [[PROF2:![0-9]+]]
+; CHECK_ICP_PROF-NEXT:    [[CALL11_I:%.*]] = musttail call ptr [[TMP1]](ptr null, i64 0, ptr null, ptr null, ptr null, ptr null), !prof [[PROF2:![0-9]+]]
+; CHECK_NO_ICP_PROF-NEXT: [[CALL11_I:%.*]] = musttail call ptr [[TMP1]](ptr null, i64 0, ptr null, ptr null, ptr null, ptr null)
 ; CHECK-NEXT:    ret ptr [[CALL11_I]]
 ;
 entry:
   %0 = load ptr, ptr null, align 8
   ret ptr null
 
 1:
   %call11.i = musttail call ptr %0(ptr null, i64 0, ptr null, ptr null, ptr null, ptr null), !prof !0