[PATCH] D120584: [NewPM][Inliner] Mark inlined calls to functions in same SCC as callee as noinline

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

In D120584#3362634 <https://reviews.llvm.org/D120584#3362634>, @nikic wrote:

> There's a couple of reasons why a default-disabled option is not great:
>
> 1. It's not compatible with linker plugin LTO. People would have to be experts on LLVM internals and know that they need to add something like `-Wl,-mllvm,-enable-unbounded-cross-scc-inlining=0` to their build system if they are linking rust object files.
> 2. This problem is not Rust-specific. According to https://discourse.llvm.org/t/rust-newpm-blocker-catastrophic-inlining/6171/2, Apple uses the earlier version of this patch, though not sure for which toolchain(s). IIRC Mozilla previously encountered this with C++ code.
> 3. While I agree that correctness and performance is just another tradeoff, we generally always trade off in favor of correctness, unless we expect a widespread performance impact, in which case may temporarily trade off in favor of performance. There is no evidence that this patch has any widespread impact, but there //is// evidence against it: For rustc performance tests, this patch is entirely performance neutral (while the previous version had minor negative impact) and the entirety of llvm-test-suite has only two programs even showing codegen changes with this patch. There might be an impact in some isolated cases (actually, this is pretty much guaranteed with any inliner change), but all evidence points towards no //widespread// impact, and as such I don't think there is justification for favoring performance over correctness.
>
>> It is reasonable to expect either 1) a proper/complete fix;
>
> I do think this is a proper fix.

There is a reason to believe this is not the proper fix: there are callsites that are perfectly ok to be inlined but will get disabled due to the way the fix is done. It is considered proper if the fix actually detects the compile time budget will be reached -- not at the first inlining.

I guess we need to agree to disagree here.

> There are some alternative ways to fix the issue which may well be better for some cases, but frankly I don't understand how you would even evaluate that without knowing about specific instances that this patch would regress. Knowing specific regressions, we could check whether an alternative patch avoids them, but without that, isn't this just guesswork?
>
>> The compile time issue is there since forever and not a recent regression.
>
> This is true, but the new pass manager has exacerbated this issue, which is a recent regression. We //did// encounter a few instances of the same issue with the legacy pass manager as well, but the issue has become much more wide-spread and pressing with the new pass manager.
>
> All that being said, I would take a default-disabled option over nothing at all. We would enable it by default in rustc, and I would give a recommendation to enable it by default in Fedora/RHEL LLVM, but at least we would not be forcing all distros to adopt the patch.

Agree -- let's make this step happen first to unblock the affected users.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120584/new/

https://reviews.llvm.org/D120584