Skip to content

Backport LLVM changes to disable deferred inlining #92110

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Dec 19, 2021

Fixes #91128

I was thinking of how to best add the test case from the issue, and I think rust perf infrastructure would probably be the best place for something like it.

@rust-highfive
Copy link
Contributor

r? @cuviper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Contributor

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2021
@nagisa
Copy link
Member Author

nagisa commented Dec 19, 2021

We want to backport either this or #91190 to beta.

@nagisa nagisa added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2021
@nagisa
Copy link
Member Author

nagisa commented Dec 23, 2021

cc @nikic

@pnkfelix
Copy link
Member

Declining to beta-backport in favor of PR #91190.

@pnkfelix
Copy link
Member

@rustbot label: -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 23, 2021
@nikic
Copy link
Contributor

nikic commented Dec 23, 2021

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Dec 23, 2021

📌 Commit 2b821f2 has been approved by nikic

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2021
@bors
Copy link
Collaborator

bors commented Dec 23, 2021

⌛ Testing commit 2b821f2 with merge 77497c7...

@Aaron1011
Copy link
Member

Note that this is an LLVM backport, not a beta backport (the PR wording confused me at first)

@bors
Copy link
Collaborator

bors commented Dec 23, 2021

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 77497c7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 23, 2021
@bors bors merged commit 77497c7 into rust-lang:master Dec 23, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 23, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (77497c7): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -8.3% on incr-patched: println builds of regression-31157)
  • Large regression in instruction counts (up to 2.8% on full builds of deeply-nested)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 23, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 1, 2022
Mark drop calls in landing pads `cold` instead of `noinline`

Now that deferred inlining has been disabled in LLVM (rust-lang#92110), this shouldn't cause catastrophic size blowup.

I confirmed that the test cases from rust-lang#41696 (comment) still compile quickly (<1s) after this change. ~Although note that I wasn't able to reproduce the original issue using a recent rustc/llvm with deferred inlining enabled, so those tests may no longer be representative. I was also unable to create a modified test case that reproduced the original issue.~ (edit: I reproduced it on CI by accident--the first commit timed out on the LLVM 12 builder, because I forgot to make it conditional on LLVM version)

r? `@nagisa`
cc `@arielb1` (this effectively reverts rust-lang#42771 "mark calls in the unwind path as !noinline")
cc `@RalfJung` (fixes rust-lang#46515)

edit: also fixes rust-lang#87055
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 1, 2022
Mark drop calls in landing pads `cold` instead of `noinline`

Now that deferred inlining has been disabled in LLVM (rust-lang#92110), this shouldn't cause catastrophic size blowup.

I confirmed that the test cases from rust-lang#41696 (comment) still compile quickly (<1s) after this change. ~Although note that I wasn't able to reproduce the original issue using a recent rustc/llvm with deferred inlining enabled, so those tests may no longer be representative. I was also unable to create a modified test case that reproduced the original issue.~ (edit: I reproduced it on CI by accident--the first commit timed out on the LLVM 12 builder, because I forgot to make it conditional on LLVM version)

r? `@nagisa`
cc `@arielb1` (this effectively reverts rust-lang#42771 "mark calls in the unwind path as !noinline")
cc `@RalfJung` (fixes rust-lang#46515)

edit: also fixes rust-lang#87055
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huge compile-time regression in beta/nightly
9 participants