LGTM. Thanks.

LGTM. Thanks.

Address Hongtao's comment.

In D94001#2495265, @wenlei wrote:

In D94001#2494594, @wmi wrote:

Sorry for the delay in review. It makes a lot of sense to have a priority based inliner for CSSPGO since its profile annotation doesn't rely on replaying the inlining. But I don't understand why we rely on BFS/DFS strategy to expose the hottest callsite for priority based inliner. In my mind, CSSPGO can know the hotness of each callsite with full context information. Can we just sort all the callsites based on CSSPGO profile then try to inline the hottest callsite under a specific context first in a top down manner? We may actually need to inline some parent callsites before we can inline the hottest callsite, but it is all driven by the target to inline the hottest callsite first. If we worry some callsite is too deep and we need to inline through a deep path before we can inline the specific callsite, we may add the depth into priority computation. What do you think?

Yeah, that's one step further than what I have in this patch. The key here is the prioritized work list, and BFS is just a natural by product of using the priority queue. I called out BFS to emphasize the more balanced inlining, but it's not super accurate because it is only a true BFS when priorities are all identical. What you suggested is essentially increasing the scope of call sites to prioritize - currently it's the immediate callees. I agreed that increasing the scope to multiple levels or the entire sub call tree may be beneficial. But I don't think it conflicts the approach in the patch, instead it can be added leveraging this implementation.

With context profiles organized in a trie, it's relatively easy to get the hottest call sites in sub call tree (assuming no stale profile). I think we can assign total order/priority for these hot call sites as well as the call sites in the middle leading to those hot ones (call sites that lead to hottest call site need to have their priority bumped up), then the priority queue will lead us to these hottest call sites first even if they're several levels away.. I can give it a try to increase the scope of call site for prioritization as a follow up.

Address Hongtao's comment.

In D94613#2496792, @hoy wrote:

Thanks for the refactoring!

BTW, looks like there is a use of ThinLTOPhase::None to be replaced in polly/lib/Support/RegisterPasses.cpp. Otherwise, LGTM.

Address Wenlei and David's comments.

In D94435#2495298, @wenlei wrote:

Thanks for the change.

If the profile is large, it is better to split the profile into two parts, one with context and one without, so the profile reading in postlink phase only has to read the part with context.

I guess the speed up is more visible with merged partial profile with explicit profile flattening? or did you see it speed up FDO with regular full profile too? For a regular FDO profile, I'd expect most function profiles to have some context except for really small functions..

I guess there will be a separate patch to turn on this work, i.e, setting CtxSplitLayout?

LGTM.

LGTM.

Sorry for the delay in review. It makes a lot of sense to have a priority based inliner for CSSPGO since its profile annotation doesn't rely on replaying the inlining. But I don't understand why we rely on BFS/DFS strategy to expose the hottest callsite for priority based inliner. In my mind, CSSPGO can know the hotness of each callsite with full context information. Can we just sort all the callsites based on CSSPGO profile then try to inline the hottest callsite under a specific context first in a top down manner? We may actually need to inline some parent callsites before we can inline the hottest callsite, but it is all driven by the target to inline the hottest callsite first. If we worry some callsite is too deep and we need to inline through a deep path before we can inline the specific callsite, we may add the depth into priority computation. What do you think?

This change speeds up this by grouping all the call frame within one LBR sample into a trie and aggregating the result(sample counter) on it.

Sorry for the delay in review. I will try to catch up.

Sorry for the delay. LGTM.

Thanks to Simon Pilgrim who helped to fix it at
https://reviews.llvm.org/rG4bb10be9a6e06a6c51cc1695ff5dc9d68c953334.

Address Hongtao's comment.

LGTM.

Address Hongtao's comment.

The const indices passed to writeOneSection were incorrect (from a stale patch). Upload the latest version.

In D93254#2453854, @wenlei wrote:

To reduce the profile loading time during postlink phase when ThinLTO is enabled, we are going to split profile into two parts. One part contains profiles with inline instance and another part contains flattened profiles without inline instance in them.

For the flattened profile, do you explicitly flatten part of raw profile, or it's just the portion of function profile that does not have any inlinee profile?

In D93254#2453629, @hoy wrote:

Can you please shed light on how multiple same-typed sections will be used? Thanks.

Please add a message for all the assertions. Other than that, LGTM.

In D92806#2443435, @hoy wrote:

@wmi Can you please recommend a reviewer for this? Thanks!

In D91878#2441363, @hoy wrote:

In D91878#2440875, @wmi wrote:

Sorry for the delay in review. Some general questions.

I remember Wenlei mentioned in the RFC discussion that the overall binary size increase from pseudo probe is ~12% if CFG encoding is added. Is the size increase for object file from pseudo probe about the same as binary? I am asking because the total size of linker input matters for us. Previously we may use fission to separate the debug information from object file so it won't be accounted as linker input.

According to the description, The address range will be split into multiple ranges by the address of pseudo probe. If some BB somehow doesn't have block type pseudo probe, the addresses in the BB will be mistakenly associated with the pseudo probe located before it in the binary. I don't know whether you saw cases like that. A random thought is we may generate some fake pseudo probes for the BBs which don't have them, as a mark of the BB boundary.

Thanks for reviewing this patch!

Regarding the binary size, an object file will likely have the same size inflation with an executable since the size of the pseudo probe sections are propositional to the text section. If that is an issue to the linker, we can also perform fission to the pseudo probe data. Pseudo probe encoding is currently done on function (comdat) level (see MCPseudoProbeSection::Emit in MCPseudoProbe.cpp`), which means a function always gets a separate real absolute probe encoded in the first place followed by relative address delta encoded for subsequent probes. Therefore fission on function level should be fine. Fission on block level won't work, as you pointed out. Would function-level fission work for you?

Sorry for the delay in review. Some general questions.

LGTM.

In D92621#2435924, @wenlei wrote:

We find that after this change, the elapsed time to build a large application distributively is reduced by 5% and the accumulative cpu time used for building is also reduced by 5%.

This is great, thanks for the patch! Just to clarify, did you mean 5% of end to end ThinLTO+AutoFDO build? I'm curious what is the total profile reading time your saw in terms of %? and how different that % is between md5 profile vs non-md5 profile? Currently we mostly use non-md5 profile, assuming md5 profile is more about saving profile size.

Address Hongtao's comment.

Address David and Hongtao's comments.

LGTM.

In D91756#2427795, @hoy wrote:

In D91756#2427759, @wmi wrote:

Another question. Sorry for not bringing it up earlier. When a call with probe metadata attached is inlined, the probe will be gone or it will be kept somehow? I think you want to keep the probe especially for inline instance to reconstruct the context but I didn't figure it out how you did that from the description.

No problem. Sorry for not clarifying it in the description. When a callee is inlined, the probe metadata will go with the inlined instructions. The !dbg metadata of an inlined instruction is in form of a scope stack. The top of the stack is the instruction's original !dbg metadata and the bottom of the stack is the for the original callsite of the top-level inliner. Except for the top of the stack, all other elements of the stack actually refer to the nested inlined callsites whose discriminator fields (which actually represents a calliste probe) can be used to represent the inline context of an inlined PseudoProbeInst or a CallInst. I'll update the description.

Another question. Sorry for not bringing it up earlier. When a call with probe metadata attached is inlined, the probe will be gone or it will be kept somehow? I think you want to keep the probe especially for inline instance to reconstruct the context but I didn't figure it out how you did that from the description.

LGTM.

LGTM.

LGTM.

LGTM.

By the way, David has gone through the patches and talked to me offline saying he is ok with the patches generally. Thanks for your great work and patience!

LGTM.

LGTM.

LGTM.