Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Disable LLVM newPM by default #91190

Closed
wants to merge 1 commit into from

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Nov 24, 2021

This is a temporary solution until we manage to get some sort of a workaround for the rampant inlining behaviour and other issues such as #91128 in place. Expect significant compilation time regressions in other areas, though.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

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

nagisa commented Nov 24, 2021

r? @nikic

@rust-highfive rust-highfive assigned nikic and unassigned oli-obk Nov 24, 2021
@nagisa nagisa force-pushed the nagisa/disable-newpm branch 2 times, most recently from 5026492 to 01f8b6c Compare November 24, 2021 19:28
@nagisa nagisa added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Nov 24, 2021
@camelid
Copy link
Member

camelid commented Nov 24, 2021

Expect significant compilation time regressions in other areas, though.

@bors rollup=never

@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 24, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_codegen_llvm/src/back/write.rs at line 400:
 ) -> bool {
     // The new pass manager is causing significant performance issues such as #91128, and is
     // therefore disabled in stable versions of rustc by default.
-    config
-        .new_llvm_pass_manager
-        .unwrap_or(false)
+    config.new_llvm_pass_manager.unwrap_or(false)
 
 
 pub(crate) unsafe fn optimize_with_new_llvm_pass_manager(
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_codegen_llvm/src/value.rs" "/checkout/compiler/rustc_codegen_llvm/src/lib.rs" "/checkout/compiler/rustc_codegen_llvm/src/base.rs" "/checkout/compiler/rustc_codegen_llvm/src/abi.rs" "/checkout/compiler/rustc_codegen_llvm/src/back/profiling.rs" "/checkout/compiler/rustc_codegen_llvm/src/consts.rs" "/checkout/compiler/rustc_codegen_llvm/src/back/write.rs" "/checkout/compiler/rustc_codegen_llvm/src/type_of.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

@nikic
Copy link
Contributor

nikic commented Nov 24, 2021

Hm, I'm not sure this is a good idea, at least for nightly. I'm afraid this is going to break more than it fixes, as we've only had isolated reports of pathological compile-time regressions.

I don't remember the details, but I'm pretty sure I've seen various nightly-only projects make changes to accommodate the NewPM switch (I think one issue was different pass naming for sanitizers or something?) and switching back and forth would be rather annoying.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_codegen_llvm/src/back/write.rs at line 400:
 ) -> bool {
     // The new pass manager is causing significant performance issues such as #91128, and is
     // therefore disabled in stable versions of rustc by default.
-    config
-        .new_llvm_pass_manager
-        .unwrap_or(false)
+    config.new_llvm_pass_manager.unwrap_or(false)
 
 
 pub(crate) unsafe fn optimize_with_new_llvm_pass_manager(
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_codegen_llvm/src/va_arg.rs" "/checkout/compiler/rustc_codegen_llvm/src/callee.rs" "/checkout/compiler/rustc_codegen_llvm/src/attributes.rs" "/checkout/compiler/rustc_codegen_llvm/src/builder.rs" "/checkout/compiler/rustc_codegen_llvm/src/back/write.rs" "/checkout/compiler/rustc_codegen_llvm/src/llvm/ffi.rs" "/checkout/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs" "/checkout/compiler/rustc_codegen_llvm/src/llvm_util.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

@nagisa
Copy link
Member Author

nagisa commented Nov 24, 2021

I tend to agree. Felix's suggestion was to disable for 1.58 now and then re-enable when we have fixes, but a less disruptive option could be simply landing an equivalent of #91189 sometime next week.

@pnkfelix pnkfelix added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Dec 2, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Dec 2, 2021

nominating for discussion two weeks from today (2021-12-16), in order to evaluate whether to land by 2021-12-23, or if the problem was addressed by more targetted fix in meantime.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 16, 2021

Current theory is that llvm/llvm-project@a8c2ba1 on LLVM's side will address #91128 in a less heavy handed way than outright disabling NewPM.

@nagisa
Copy link
Member Author

nagisa commented Dec 19, 2021

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

@nagisa nagisa added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 19, 2021
@pnkfelix
Copy link
Member

Discussed in T-compiler meeting. We'll beta-backport this for now, and let #92110 (which as of this comment has not yet landed) bake on nightly for a little while.

@rustbot label: +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 23, 2021
@bors
Copy link
Contributor

bors commented Dec 30, 2021

☔ The latest upstream changes (presumably #91125) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 5, 2022
@Mark-Simulacrum
Copy link
Member

@nagisa this is targeting master -- but hasn't landed yet there. It looks like some of the discussion indicates we may want to only land this on beta?

It merges cleanly onto beta, so I'm going to backport it there for now.

@nagisa
Copy link
Member Author

nagisa commented Jan 5, 2022

Yeah, beta only. We merged a proper fix to master.

Originally this was intended for master as well, but is no longer necessary.

@Mark-Simulacrum
Copy link
Member

OK, I'll go ahead and close this PR then -- backport in #92592.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2022
…ulacrum

[beta] backports

Backports these PRs:

* Fix HashStable implementation on InferTy rust-lang#91892
* Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking rust-lang#91870
* Make rustdoc headings black, and markdown blue rust-lang#91534
* Disable LLVM newPM by default rust-lang#91190
* Deduplicate projection sub-obligations rust-lang#90423
*  Sync portable-simd to remove autosplats rust-lang#91484 by dropping portable_simd entirely (keeping the subtree, just from std/core)
*  Quote bat script command line rust-lang#92208
* Fix failing tests rust-lang#92201 (CI fix)

r? `@Mark-Simulacrum`
@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.