LGTM.

LGTM.

The patches split from the main one look good to me. Please see if David has further comments.

In D87740#2277446, @hoy wrote:

Thanks for the fix! LGTM.

I'm also seeing InstrProfSymtab is there to handle MD5-based names for PGO instrumentation. Not sure how it is used exactly. @wmi Have we thought about using that for AutoFDO?

LGTM.

LGTM

In D86495#2264877, @hoy wrote:

In D86495#2264868, @wmi wrote:

It is better to have a MIR test.

Yeah, the test is actually included in https://reviews.llvm.org/D86499. It tests both the IR and MIR change.

It is better to have a MIR test.

In D86502#2245578, @hoy wrote:

In D86502#2245460, @wmi wrote:

The early instrumentation also allows the inliner to duplicate probes for inlined instances. When a probe along with the other instructions of a callee function are inlined into its caller function, the GUID of the callee function goes with the probe. This allows samples collected on inlined probes to be reported for the original callee function.

Just get a question from reading the above. Suppose A only has one BB and the BB has one PseudoProbe in it. If function A is inlined into B1 and B2 and both B1 and B2 inlined into C, the PseudoProbe from A will have two copies in C both carrying GUID of A. How the samples collected from A inlined into B1 inlined into C are categorized differently from A inlined into B2 inlined into C, especially when debug information is not enabled (so no inline stack information in the binary)?

This is a very good question. Inlined functions are differentiated by their original callsites. A pseudo probe is allocated for each callsite in the SampleProfileProbe pass. Nested inlining will produce a stack of pseudo probes, similar with the Dwarf inline stack. The work is not included in the first set of patches.

The early instrumentation also allows the inliner to duplicate probes for inlined instances. When a probe along with the other instructions of a callee function are inlined into its caller function, the GUID of the callee function goes with the probe. This allows samples collected on inlined probes to be reported for the original callee function.

In D86193#2240502, @hoy wrote:

In D86193#2240353, @wmi wrote:

There are some optimizations such as if-convert, tail call elimination, that were initially blocked by the pseudo probe intrinsic but is now unblocked by fixes included in this change. With the current change we do not see perf degradation out of SPEC and one of our internal large services.
The main optimizations left blocked intentionally are those that merge blocks for smaller code size, such as tail merge which is the opposite of jump threading. We believe that those optimizations are not very beneficial for performance and AutoFDO.

If the optimizations are not very beneficial for performance and AutoFDO and should be blocked, it may be better to block them in a more general way and not depend on pseudo probe, because blocking them may also be beneficial for debug info based AutoFDO.

In theory, yes, we should have a black list of transforms (mainly related to block merge) that are not needed by AutoFDO and block them. In reality it might take quite some efforts to figure them out. Pseudo probe, on the other hand, starts with blocking those transforms in the first place and relax the ones that might actually help AutoFDO.

Another reason is that pseudo probe looks pretty much like debug information to me. They are used to annotate the IR but shouldn't affect the transformation. Binaries built w/wo debug information are required to be identical in LLVM. I think that requirement could be applied on pseudo probe as well. It is even better to have some test to enforce it so that no change in the future could break the requirement.

Good point! Yes, pseudo probe is implemented in a similar way with the debug intrinsics. However they are not guaranteed to not affect the codegen since its main purpose is to achieve an accurate profile correlation with low cost. Regarding the cost, it sits somewhere between the debug intrinsics and the PGO instrumentation and close to a zero cost in practice.

There are some optimizations such as if-convert, tail call elimination, that were initially blocked by the pseudo probe intrinsic but is now unblocked by fixes included in this change. With the current change we do not see perf degradation out of SPEC and one of our internal large services.
The main optimizations left blocked intentionally are those that merge blocks for smaller code size, such as tail merge which is the opposite of jump threading. We believe that those optimizations are not very beneficial for performance and AutoFDO.

Thanks for splitting the patch into smaller ones. Can you have a separate test just to check the pseudo probe can be parsed and not deleted by any pass?

Thanks Wenlei for the review and the insightful suggestions!

Address Wenlei's comment.

Address Wenlei's comment and add the support to search indirect call promotion candidate using remapper.

I don't know since I don't use arc.

Thanks for the patch! A few questions:

Address Hongtao's comment.

Hongtao, thanks for the review!

Remove an unrelated file being accidently included.

Thanks. Another suggestion about comment. Other than that, LGTM.

LGTM. Thanks.

I see. Thanks for the explanation.

Commit the fix for the second problem in D84997 together with the assertion and testcase in this patch, so D84997 can be dedicated to solve the other problem.

The multiple inline instance copies generated from loop unrolling or jump threading should have their own FunctionSamples objects. Why merging multiple inline instance to the outline profile will trigger assertion? Could you have a testcase for it?

It's a slightly difference case. In the training build, we didn't unroll, so there's a single inline site in the loop, but in the PGO optimizing build, the loop got unrolled with call site duplicated. And the duplicated call sites all point to the same FunctionSamples, in which case we should merge them only once. Agree that we should add a test case.

In D84997#2186522, @wmi wrote:

Yes, the fix in D84997 is more concise. Here I use a straightforward fix. It is ok for Hongtao to commit D84997, but just add a testcase (free to use the one here).

Yes, the fix in D84997 is more concise. Here I use a straightforward fix. It is ok for Hongtao to commit D84997, but just add a testcase (free to use the one here).

Yes, the fix in D84997 is more concise. Here I use a straightforward fix. It is ok for Hongtao to commit D84997, but just add a testcase (free to use the one here).

The multiple inline instance copies generated from loop unrolling or jump threading should have their own FunctionSamples objects. Why merging multiple inline instance to the outline profile will trigger assertion? Could you have a testcase for it?

Thanks for the work. It is a very useful feature.

Address David's comments.

Address David's comment

I think this feature should be decoupled from the version change -- since this is an approximate anyway. One way to do this is to use max count or total count as a reference point and compute the scale factor.

I also plan to dump the functions cold in instr profile and hot in sample profile, and sort the output according to hotness in sample profile. That can be used to guide PGO users if they want to improve the representativeness of their loadtest. I leave that part in a separate patch for easier review.

LGTM. Better wait another day to see if other reviewers have further comments.

Thanks for the explanation.

In D83743#2150725, @wenlei wrote:

In D83743#2150703, @davidxl wrote:

Can this be extended to the SCC inliner?

Yes, we can use it in SCC inliner as well. We just need extra plumbing there. We can do that in a separate change if needed.

Address David's comments.

In D79978#2141959, @MaskRay wrote:

In D79978#2141768, @wmi wrote:

In D79978#2140849, @MaskRay wrote:

I haven't looked into the details, but the test suggests that the patch is wrong:

# basic-block-sections-cfi-1.ll
        .section        .text,"ax",@progbits,unique,2
_Z2f3b.2:                               # %if.end
        .cfi_startproc
        .cfi_def_cfa %rbp, 16    # this should be inserted after addq $16, %rsp
        .cfi_offset %rbp, -16    # this should be after .cfi_def_cfa %rbp, 16
        addq    $16, %rsp
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq

I think the position where the cfi directives are currently inserted is correct. Those directives at the beginning of BB are not to maintain call frame information for instructions inside of BB like "addq $16, %rsp" and "popq %rbp", but to setup the call frame information correctly at the beginning of BB because the BB could be moved around.

Ack. Then what instructions should be placed at the top of these basic blocks? Should .cfi_def_cfa_register %rbp be placed as well? If you move these basic blocks around, .cfi_def_cfa_register %rbp is currently not tracked.

In D79978#2140849, @MaskRay wrote:

I haven't looked into the details, but the test suggests that the patch is wrong:

# basic-block-sections-cfi-1.ll
        .section        .text,"ax",@progbits,unique,2
_Z2f3b.2:                               # %if.end
        .cfi_startproc
        .cfi_def_cfa %rbp, 16    # this should be inserted after addq $16, %rsp
        .cfi_offset %rbp, -16    # this should be after .cfi_def_cfa %rbp, 16
        addq    $16, %rsp
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq

Fix a wrong flag name in test.

Address David's comment.
Adjust comments, function names and flag names.

refactor GetEntryForPercentile out in https://reviews.llvm.org/D83439

https://reviews.llvm.org/D82123 to always instrument function entry BB has been committed guarded by a flag. https://reviews.llvm.org/D83024 to enable the flag by default is under review.

Thanks. LGTM. Please wait and see if David has more comments.

Disable sample-profile-merge-inlinee when sample-profile-top-down-load is not effective (Currently sample-profile-top-down-load is only effective for new pass manager).

I tested the performance with sample-profile-top-down-load and sample-profile-merge-inlinee both enabled. In different compiler versions I got different result. In one version about three weeks older, I got 0.4% improvement for one benchmark steadily in multiple runs and neutral for another. In the head llvm version, I saw neutral result for both benchmarks.

Enable -sample-profile-merge-inlinee by default together with -sample-profile-top-down-load.

In D83024#2132133, @davidxl wrote:

Here version is overloaded with different meanings: 1) to indicate format change; 2) to indicate instrumentation compiler version that is capable of producing this format. These two purposes can be contradicting to each other as the user can use option to force the old format with the new compiler.

Sounds good to me. I will redo the performance test for it.

Thanks David!

Remove the compiler part since that part will be done in https://reviews.llvm.org/D82123.

I tried it on an internal benchmark.

LGTM. Thanks.

Thanks for the patch! I guess atomic update is to reduce the cost of the entry counter for application with many threads. How much difference there is w/wo enabling -atomic-first-counter?

In D81981#2099769, @davidxl wrote:

The index format needs to be backward compatible, so there needs to be some version specific handling there (can be removed later).

In D81981#2099702, @wenlei wrote:

If how we select BB to instrument depends on a switch, we would need instrumentation build and optimizing build to have consistent switch, otherwise counters could mismatch even if CFG checksum matches? I guess that's one reason why it'd be good to avoid different ways of selecting BBs.

In D81981#2099452, @davidxl wrote:

Why is the profile size increase? I expect the number of instrumented blocks remain mostly unchanged.

The reason for the question is that if the overhead is low, I think we should make the default to be true.

In D81981#2099070, @davidxl wrote:

I think it is good to have an entry counter always, so that the profile dump is more readable. Do you have data showing the instrumentation overhead and profile size impact (clang and some large app)?

Thanks for adding the feature.

Based on my limited knowledge of new pass manager, this change looks good. Please wait another day or two before commit in case Chandler has some comment.