LGTM

Thanks for the fix.

Looks good to me. Thanks for fixing.

Lgtm.

Looks good to me.

integrated David's review comments

This new patch integrated David's review suggestion to make this optimization a separated pass.
This pass will be called after inlining to avoid the potential interference to in-lining decisions. It will use both the metadata annotation for memop size profiling and the enclosing BB's edge count to find the actual count.

Integrated David's suggestion to make the optimization a stand along pass (a function pass).
It currently resides in IndirectCallPromotion.cpp. I will have a follow-up patch to change IndirectCallPromotion.cpp to a more appropriate name.

randomize the order the VP profile entry in proftext, as suggested by David.

Fix the testcase.

Integrate David's comments. Using a separated option to control the number of annotation for memop_size.

In D28965#701857, @davidxl wrote:

After the refactoring code lands, do you plan to add the site annotation part in this patch?

Update the patch: remove some unrelated changes.

This is the patch that after splitting out the llvm-profdata refactor change.

In D28966#694902, @davidxl wrote:

The optimization pass should be split into two phases as IC promotion. The annotation part should probably be merged with the instrumentation patch. The transformation patch should be done in the same pass as IndirectCallPromotion.

Integrated David's review comments.

Updated the patch according on David's review comments.

Not familiar with samplePGO. Are the counters always annotated in the call sites (or the sampled BB)?
The call counts cannot be inferred from entry counts and the branch-weight?
If this the way samplePGO works, this looks fine to me.

In D29512#666652, @mehdi_amini wrote:

From the description:

Current internal option -static-func-full-module-prefix strips all the directory path the profile counter names for static functions. The default of this option is true.

...

This patch turns the default value for -static-func-full-module-prefix to false.

However in the diff I see:

• "static-func-full-module-prefix", cl::init(false), + "static-func-full-module-prefix", cl::init(true),
  
  Which is the opposite of the description? This seems confusing to me.

Update the patch based on the reviews:
(1) move the code to ProfileData to make the change local to profile
(2) add negative test
(3) add a comment on how it affects thin-lto.

Integrated David's review comments.

Add a testcase

LGTM too.

why limit to merge mode?
non-merge mode also exhibits the problem mentioned.

move platform depended code to InstrProfilingUnil.c per David's suggestion.

updated the option description per Teresa's suggestion.

thanks to Teresa for the collection. I'll update the patch shortly

Look good to me.

Integrated David's review comments.

Integrated part of vsk's review comments.

To vsk: if seems hard to add a test without the compiler part of change. We can add the calls to __llvm_profile_instrument_range(). But to check the result, we need llvm-profdata to dump the counters.

One reply to vsk's comment in inlined.

drop the recent change in InstrProfData.inc

It seems I have to fall back the latest change in InstrProfData.inc. We have to use #else clause "VALUE_PROF_FUNC_PARAM(uint32_t, CounterIndex, Type::getInt32Ty(Ctx))" because one of the version need to append INSTR_PROF_COMMA.

Integrated David's review comments.

This gonna be very useful in debug PGO related issues -- we don't need to use the instrumentation debug dumps to track the counts (which is not available in debug builds).
Overall, the patch looks good to me except the minor issues (see inline comments).

Update the patch based on David's review comments.

changed the patch based on David's review comments:
(1) reuse __llvm_profile_instrument_target as much as possible and make the new function a wrapper.
(3) use parameter rather global variables.
(2) supported signed type.

This change gonna hide the missing profile for a function permanently if there is no mismatch. Like, if a user adds a new function without changing existing functions, he will never get a warning if using the old profiles.