In D76248#1931542, @hoyFB wrote:

In D76248#1931511, @wenlei wrote:

Thanks for fixing this! I forgot to ask earlier, but this could happen with inlining under new PM as well, right? With new PM, we collect calls within SCC, then iterate over them for inlining, so for the same reason we could have dangling call in VMap from cloning?

Yes, it could, though that didn't happen in our case. I think it's due to different orders calls are processed in the legacy and new inliner.

Thanks for fixing this! I forgot to ask earlier, but this could happen with inlining under new PM as well, right? With new PM, we collect calls within SCC, then iterate over them for inlining, so for the same reason we could have dangling call in VMap from cloning?

Great to MD5 support added to ext-bin, thanks @wmi. After this change it seems compact-bin is completely superseded by ext-bin, do you plan to deprecate/remove it?

Thanks for clarification, LGTM!

Nice change, thanks @junparser! The improved dataflow analysis using lifetime instead of allocation point and the resulting shrink-wrapping style opt for alloca would definitely be beneficial.

This change broke our use of LLVMSupport library, and I also noticed it's been reverted already.

I'm not sure I understand the motivation for this change. Can you please expand on that? How are you using it that it doesn't work for you?

Thanks for the patch, @junparser! Curious how did you notice this - was it causing performance regression for your workload?

@jdoerfert good to know it's being fixed - thanks! I saw that fix is accepted, do you plan to land it now, asking because this broke dozens of internal builds, so we want to resolve it asap - deciding whether to wait for your fix to go in or we need to port over your fix manually.

Committed on behalf of @hoyFB per request.

Thanks for the making the changes, it would be nice to have some consistency (the same structure) between test cases for legacy PM and new PM, e.g. EMPTY is only tested for legacy PM, but not new; multi-function filter also only tested for legacy PM, would be nice to do them for both, and unify the check-prefix, otherwise LGTM.

Sounds good. I was trying to figure out how to invoke the new pass manager with OPT. Is there a command line switch to do that?

Thanks for fixing this. Could you use .ll IR file as test case and place it alongside with test/Other/module-pass-printer.ll or just reuse that one? We can also use opt to invoke new pass manager, so it doesn't need to be a clang test.

LGTM. Thanks for fixing the coro cleanup issue with ThinLTO here.

Comment needs to be updated, otherwise LGTM.

Sorry for the delay, LGTM except the file comment as Johannes pointed out. We've tested this stack of coroutine patches with multiple large services internally, all working as expected, so I'm going to accept this stack as review feedbacks are addressed too.

LGTM. We've tested this stack of coroutine patches with multiple large services internally at facebook, all working as expected.

Is that the desired behavior? This was the previous behavior but I'm not sure it's the right behavior.

@aganea, @hans This patch broke response file expansion for preprocessor via -Wp - all our internal builds failed with this patch because we invoke clang with response file passed to preprocessor, e.g. clang++ ... -Wp,@pp.rsp @cc.rsp. Looks like we can't just call ExecuteCC1Tool directly, because response file expansion happens in main, so with this patch, -Wp,@pp.rsp won't be expanded properly.

In D72490#1813569, @mgorny wrote:

Is there a way to get the basename from ZLIB_LIBRARY by any chance? Not saying it's actually necessary, just wondering.

Thanks for making the changes to schedule 2nd coro-split so funclets get fully optimized. A few comments inline, looks good otherwise.

The short-circuit change makes sense to me. But what caused the creation of extra SCCs due to self-recursion? Is that because of the short-cut from line 808-825 and the clearing of SCC stack and DFS stack broke the LowLink and DFSNumber check of later Tarjan DFS?

The devirt trigger here is to restart CGSCC pipeline to run coro-split again to split the coroutine function. There is no need to introduce devirt trigger in NewPM since we call coro-split pass manually.

My understanding is devirt trigger is only a trick used with Legacy PM to run optimizations on coroutine funclets after coro-split. With NewPM's CGSCCUpdateResult infra, can we communicate that change and request passes directly with ModuleToPostOrderCGSCCPassAdaptor, without artificially introducing the indirect call and devirt?

address feedback

rebase

update test case

Restructured remarks sample profile loader inlining:

1. Split into InlineAttempt, InlineSuccess and InlineFail for remark names.
2. Use OptimizationRemark only for InlineSuccess and OptimizationRemarkAnalysis for the rest.

Did performance test and I saw 0.4% improvement in an internal benchmark. That is a good improvement, thanks for the change!

address feedback

address feedback

address review feedback - only do fuzzy match for call site targets for indirect calls, as a result, I had to fix a few test cases.

In D70653#1761978, @wmi wrote:

I did performance test combing D70653 and D70655, there was ~0.2% latency improvement in a benchmark, so that is good.

update test case

address review feedback

I would imagine among all the functions with inline instance in profile, only those small and warm/cold functions which are not inlined in early inliner will be inlined in regular inliner.

Thanks for the discussions and code review. I'll address the comments later, but wanted to answer this one first.

The only concern is for inline instance getEntrySamples is not precise and sometimes can have a large difference with the actual head count

In D70655#1759165, @davidxl wrote:

One way to handle it is 1) delay early inlining of sites into a hot function if a big percentage of calls to the function are from other modules. This still allows intra module top down inlining of it; or 2) keep a clone of the unlined body of the original function and use that one during cross module inlining.

Can this be handled for cross module (thinLTO) case somehow too?

In D52845#1707286, @davidxl wrote:

Wenlei, this sounds like a good idea. Patches are welcome!

In D69736#1734892, @wmi wrote:

I saw 0.3% improvement in our server benchmark. That is a great improvement!

Split UpdateProfile part, address other feedbacks.

update the right version of setEntryCount

address review feedback

I agree setEntryCount always copying the existing import list is better than adding updateEntryCount. Adding a separate metadata also sounds good to me. I don't know the history why enbedding import list in function_entry_count. @tejohnson may know.

The comments indicate that this is in part due to issues with
the new PM loop pass manager

This is a minimal point fix (was trying to keep internal patch minimum), but it might actually make sense to have setEntryCount always copy over the existing import list (if it exists), instead of adding updateEntryCount? Or even further, we could have a separate metadata for the import list, instead of piggyback on function_entry_count? To me, it seems a bit weird to embed import list as part of function_entry_count - wondering what's the reason behind that?

Thanks for making the changes, LGTM!

@wmi @davidxl FYI, we have an internal patch to address this issue further. This patch only scales up entry count of outline function for cold callsites, thus losing context-sensitiveness. But ideally, we could reuse the entire profile from that inline context, and merge it all back to the outlined function. We changed the function annotation/inlining order to be top-down, then as we decide to not inline a cold call site, we merge the entire FunctionSamples of that inlinee back to outline counterpart, and since we process functions in top-down order (ignoring recursion for a second), annotation of the outline callee is able to use post merge FunctionSamples which is more accurate than simple scaling of entry count. (Doing context-sensitive inlining top-down has other benefits too, as it allows specialization whiling inlining which helps maximize the benefit of having context sensitive profile)

Symbol list can be provided to llvm-profdata in a plain text file.

Thanks for making indexing available for extended binary format. We've recently adding indexing to binary format as well in an internal patch, and also observed similar (~30%) build time reduction for some large services.

Before:

...
  13.1: inlined callee: _ZN6StringD2Ev: 0, 0, 0 sampled lines
    No samples collected in the function's body
    Samples collected in inlined callsites {
      0: inlined callee: _ZN6String4freeEv: 0, 0, 6 sampled lines
        Samples collected in the function's body {
          2: 0
          4: 0
          5: 0
          6: 0
          7: 0
          8: 0
        }
        Samples collected in inlined callsites {
          6: inlined callee: my_free: 0, 0, 1 sampled lines
            Samples collected in the function's body {
              4: 0
            }
            No inlined callsites in this function
}
}
}

After:

...
  13.1: inlined callee: _ZN6StringD2Ev: 0, 0, 0 sampled lines
    No samples collected in the function's body
    Samples collected in inlined callsites {
      0: inlined callee: _ZN6String4freeEv: 0, 0, 6 sampled lines
        Samples collected in the function's body {
          2: 0
          4: 0
          5: 0
          6: 0
          7: 0
          8: 0
        }
        Samples collected in inlined callsites {
          6: inlined callee: my_free: 0, 0, 1 sampled lines
            Samples collected in the function's body {
              4: 0
            }
            No inlined callsites in this function
        }
    }
}