Landed the change on behalf of @wlei.

LGTM except a minor comment.

LGTM. A small nit: the code snippet in the description could be removed as it's indeed in the codebase already. :)

Just saw this. Thanks @Alina Sbirlea for fixing the tests, much appreciated!

Committed on behalf of @ayermolo.

Committed on behalf of @modimo.

Committed on behalf of @modimo.

I expect static and dynamic counts in general will require rounds of tuning and will still hit pathological cases on certain scenarios.

Chatted with @lxfind off patch in the morning. A quick summary:

Landed on behalf of @ayermolo ..

LGTM. I'd recommend wait a few days for other reviewers.

Took another look, AlwaysInline is part of LLVMipo too. :(

LGTM. @tejohnson appreciate if you could take a look too, thanks. (this is blocking an internal usage for us)

In D85812#2243570, @dongAxis1944 wrote:

In D85812#2237812, @wenlei wrote:

In D85812#2234313, @MaskRay wrote:

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

if i put the attribute CORO_PRESPLIT_ATTR to the attribute.td, it seem it might not have the layering issue(Because it will not depend coroutine lib itself), do you think it might be better?

In D77925#2236519, @tejohnson wrote:

In D77925#2229484, @mehdi_amini wrote:

Overall that would likely work for XLA. Something I'd like to mention though in response to:

The veclib type is also tied to the accepted values for -fveclib, which is a list of supported lib,

-fveclib is a Clang thing, it shouldn't limit what LLVM does. Of course LLVM needs to support Clang, but does not have to inherit the limitation of map 1:1 to Clang UI.
In particular as a library, it isn't clear why we would make the choice to write LLVM VecLib support this way.

In D86156#2243393, @asbirlea wrote:

Diff looks reasonable at this point. Thank you for the patch!
Please wait on @nikic for compile-time impact or additional feedback.

Just out of curiosity, in D65060, it was mentioned that using BFI got you ~7% improvement for a CPU related metric (@wenlei). Are you seeing benefits from this patch? And which pass manager are you using?

In D85812#2234313, @MaskRay wrote:

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

LGTM, thanks!

Great - thanks for making the changes. Looks good with two minor comments.

If only LICM benefits from it, I'd consider creating a BFI instance for the duration of the LICM pass

In D77925#2220177, @wenlei wrote:

In D77925#2220169, @mehdi_amini wrote:

I rather have a non closed list of veclib: if you just have a map keyed by string instead of an enum it should just work out?

The veclib type is also tied to the accepted values for -fveclib, which is a list of supported lib, anything outside of the list is rejected with error. If you want to allow arbitrary strings as key, it's inconsistent with the restricted/closed values for -fveclib. So basically there's no openness from clang/llvm tool, while there was some openness through the libraries. I think by introducing a "Custom" veclib type, we can solve the impedance mismatch there. And in the XLA case, conceptually it's an indeed a custom veclib, right? Functionality-wise, Custom should just work for XLA usage too.

LGTM, thanks for the fix.

LTGM too. Thanks for the fix!

In D77925#2220169, @mehdi_amini wrote:

I rather have a non closed list of veclib: if you just have a map keyed by string instead of an enum it should just work out?

In D77925#2016326, @tejohnson wrote:

In D77925#2016299, @wenlei wrote:

@mehdi_amini @tejohnson When can we re-land this (with tweaks)? I'm under the impression that a test case demonstrating the 3rd party usage will be added very soon after this revert, then we can tweak the original patch to accommodate that usage, and re-land asap. Or am I missing something there? I'd like to get this unblocked asap. Currently we have to keep this as a private patch on our end which is a bit cumbersome, and I think this one can be useful for others too. Thanks..

@bkramer can you work with Wenlei on this (original patch is D77632).

@wenlei, in the meantime you can see the use case here:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/service/cpu/compiler_functor.cc#L198
for revising the patch.

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

You'd also want to prevent inlining presplit coroutine in sample profile loader (SampleProfileLoader::inlineHotFunctions), which is before CGSCC pipeline and does top-down inlining now.

For my 2nd question, this patch is what I think how this should be done: https://reviews.llvm.org/D79485

I have two questions for this patch:

Looks like some CV/COFF tests need to be updated too?

Thanks. LGTM.

Thank you for working on this during internship, @weihe! The extra tweaks here on top of our internal version look good to me as well. The test failure doesn't seem related, I will rebase and land this on your behalf.

In D84997#2186553, @wmi wrote:

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.

Thanks. So it is a combined version of normal PGO and sampleFDO, or the PGO here means the instrument hook for sampleFDO?

In D84997#2186518, @wmi wrote:

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?

Just realized this is fixing the same issue as @hoyFB's D84997 - we ran into it immediately after -sample-profile-merge-inlinee is turned on too.

LGTM. Thanks!

In D84715#2186299, @hoyFB wrote:

In D84715#2179099, @weiwang wrote:

In D84715#2177627, @wenlei wrote:

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

If we want passes to absorb such empty functions, then there needs to be consistency for all passes. I don't think passes need to absorb such cases though, as this is really a "limbo state" of functions when being removed (a normal empty function should still have entry block and ret instruction).

Agree. The proposed checks are only placed along the particular call path that leads to the failure. If we are going cover all the analyses/passes, the overall effort could be substantial.

If the big assumption is that there shouldn't be an empty body function to deal with, we can probably change how ORE is created at the beginning. Instead of creating a heavy-weight version of ORE (with all dependent analyses invoked), using the light-weight version OptimizationRemarkEmitter ORE(F, nullptr) at LTO.cpp:801.

Can we bail out for dead/empty functions right there before launching ORE on them?

Committed on behalf of @christylee while she tries to recover commit access.

I'm guessing these are not the only passes that will choke with empty/dead functions. If this is because ORE is being used for a special case, then we might want to fix the special case on the user side.

This is a resubmit for https://reviews.llvm.org/D83743.

test

test

test

In D83743#2159588, @stellaraccident wrote:

The inline-replay.ll test that this added appears to be broken (as was noted by the pre-merge check https://reviews.llvm.org/harbormaster/unit/view/120108/) and is failing in our build.

In D83743#2156697, @mtrofin wrote:

lgtm - a small nit, could you also add in the description the current scope (the top down, SampleProfile case) - for clarity. Thanks!

address feedbacks.

rebase

LGTM. Thanks!

In D83743#2153532, @wmi wrote:

Thanks for the explanation.

We could also replay CGSCC inline of one build in another. The way it's implemented would work for those scenarios.

When you replay CGSCC, the inline decision in replay file should have order? If it is unordered, I cannot see how it can replay CGSCC inline from another build exactly.

Reserve space in a vector to reduce the number of mallocs. The size of VTList is usually 1, but often 6 or 7.

Address feedback, update comment.