Improve support for NewPM #83894

nikic · 2021-04-05T15:49:08Z

This adds various missing bits of support for NewPM and allows us to successfully run stage 2 tests with NewPM enabled.

This does not yet enable NewPM by default, as there are still known issue on LLVM 12 (such as a weak fat LTO pipeline). The plan is to make the switch after we update to LLVM 13.

rust-highfive · 2021-04-05T15:49:11Z

r? @estebank

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

nikic · 2021-04-05T15:49:33Z

@bors try @rust-timer queue

rust-timer · 2021-04-05T15:49:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-04-05T15:49:41Z

⌛ Trying commit 981744e31838ae78a243bbf928fcd1a876a3864a with merge b1b723b5dc62feaeb9a176d276f190f0aa045d0a...

bors · 2021-04-05T16:37:46Z

☀️ Try build successful - checks-actions
Build commit: b1b723b5dc62feaeb9a176d276f190f0aa045d0a (b1b723b5dc62feaeb9a176d276f190f0aa045d0a)

rust-timer · 2021-04-05T16:37:47Z

Queued b1b723b5dc62feaeb9a176d276f190f0aa045d0a with parent 39eee17, future comparison URL.

rust-timer · 2021-04-05T18:30:52Z

Finished benchmarking try commit (b1b723b5dc62feaeb9a176d276f190f0aa045d0a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

Mark-Simulacrum · 2021-04-05T18:36:25Z

Very nice results over all, but some pretty major regressions on the self-bootstrap tests:

Mark-Simulacrum · 2021-04-05T22:52:05Z

No? We run with -j1, so the bootstrap should be single threaded. I'm not sure, but I seem to recall the total being computed client side in the UI - would be worth checking that, I suppose.

the8472 · 2021-04-05T23:08:20Z

Nevermind, I eyeballed it wrong. I checked with a spreadsheet now and everything is consistent.

estebank · 2021-04-08T18:33:37Z

Some really nice improvements! The code looks fine, but I am not the most qualified person for this part of the codebase. @rust-lang/compiler anyone can take on this review?

nagisa · 2021-04-08T18:38:24Z

r? @nagisa

nagisa

Overall this LGTM implementation wise. A couple comments inline. r=me after those are resolved.

I wish we had a better explanation for why we could drop some of the older workarounds (e.g. forcing of -O1) and an explanation of how things break with -opt-bisect-limit=0 in the commit message(s).

compiler/rustc_codegen_llvm/src/back/write.rs

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp

nikic · 2021-04-10T16:10:42Z

I wish we had a better explanation for why we could drop some of the older workarounds (e.g. forcing of -O1) and an explanation of how things break with -opt-bisect-limit=0 in the commit message(s).

I've made those commit messages a bit more detailed. For -opt-bisect-limit=0 the issue is an assertion failure because NameAnonGlobals ends up being skipped as well. That might be an LLVM bug, in that the pass should probably be marked as required.

For -O1 the issue is that NewPM performs inlining at that level, which breaks tests that assume -O0 -lto=thin does essentially no optimization. Unfortunately I don't know why it is fine to drop the special handling now -- I'm assuming it is fine because no tests break if we do.

nagisa · 2021-04-10T17:35:22Z

compiler/rustc_codegen_llvm/src/back/write.rs

    );
+    result.into_result().map_err(|()| llvm_err(diag_handler, "Failed to run LLVM passes"))


I think this Failed wants to be non-capitalized. Ends up as error: Failed… now which looks out of place.

nagisa · 2021-04-10T17:36:36Z

r=me with non-capital f.

nikic · 2021-04-10T19:47:05Z

@bors r=nagisa rollup=never

bors · 2021-04-10T19:47:06Z

📌 Commit 4903c075fdb6de4b0eba1461de6dae4a2b1b6d0a has been approved by nagisa

bors · 2021-04-10T21:41:38Z

⌛ Testing commit 4903c075fdb6de4b0eba1461de6dae4a2b1b6d0a with merge 07f00298b1780075a702e0d3926cf0fd2aec8eed...

cuviper · 2021-04-28T00:34:17Z

FYI, a few days ago Arthur proposed legacy PM deprecation:

https://lists.llvm.org/pipermail/llvm-dev/2021-April/150100.html

We should have a deprecation timeline that revolves around the major releases. I'd say we announce the legacy PM for the optimization pipeline to be deprecated the major release after the new PM has been the default. Looks like the new PM switch made it into LLVM 12.

He was corrected that the switch will only happen in 13, so that would mean it could be announced deprecated in 14, and I suppose it may start getting crippled or even removed soon after. Regardless, that's more emphasis that we should definitely switch rustc when we get to 13.

bors · 2021-04-28T01:16:56Z

⌛ Testing commit fdacbd2574fb891e2ba1a19615ae6ed2dac26b53 with merge 028fe2358ac144ac35144a9e2e31dfe00a0cbdc0...

bors · 2021-04-28T02:18:15Z

💔 Test failed - checks-actions

nikic · 2021-05-07T22:21:04Z

Reduction of the assertion failure:

; RUN: llc -O0
target triple = "aarch64-unknown-linux-gnu"

define void @test() {
  switch i128 0, label %label1 [
    i128 1329227995784915872903807060280344576, label %label1
  ]

label1:
  ret void
}

Which is already fixed by llvm/llvm-project@c2b322f and backported in llvm/llvm-project@fa0971b -- and which is the first 12.x commit not in our fork, duh.

To allow it to have an LLVM version dependent default.

Don't use "passes" for this purpose, explicitly insert it into the correct place in the pipeline instead.

This causes an assertion failure under NewPM, because it also ends up disabling the NameAnonGlobals pass. Instead pass -Copt-level=0 to disable optimizations. If that should be insufficient, we can use -C no-prepopulate-passes.

This doesn't seem to be necessary anymore, although I don't know at which point or why that changed. Forcing -O1 makes some tests fail under NewPM, because NewPM also performs inlining at -O1, so it ends up performing much more optimization in practice than before.

And report an error if parsing the additional pass pipeline fails. Threading through the error accounts for most of the changes here.

This updates the LLVM submodule with recent LLVM 12.x fixes. In particular, it resolves an assertion failure when targeting AArch64 at O0.

nikic · 2021-05-08T09:03:43Z

I've merged current upstream 12.x into our fork, and have added the submodule update here. That should resolve the AArch64 assertion failure.

nagisa · 2021-05-09T14:58:18Z

@bors r+

bors · 2021-05-09T14:58:19Z

📌 Commit 1b928ff has been approved by nagisa

bors · 2021-05-09T16:19:26Z

⌛ Testing commit 1b928ff with merge 7a2f446...

bors · 2021-05-09T19:00:59Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 7a2f446 to master...

nikic · 2021-08-21T20:20:18Z

Hrm, it looks like the wasm-panic-small test still fails even with the LLVM 13 upgrade:

LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small  foo.rs -C lto -O --target wasm32-unknown-unknown --cfg a
wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm
805
[ "`wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm`" -lt "1024" ]
LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small  foo.rs -C lto -O --target wasm32-unknown-unknown --cfg b
wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm
15687
[ "`wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm`" -lt "5120" ]

rust-highfive assigned estebank Apr 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2021

rust-highfive assigned nagisa and unassigned estebank Apr 8, 2021

nagisa approved these changes Apr 8, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/back/write.rs Outdated Show resolved Hide resolved

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp Show resolved Hide resolved

nikic force-pushed the newpm branch from 981744e to 2856d04 Compare April 10, 2021 16:02

nagisa reviewed Apr 10, 2021

View reviewed changes

nikic force-pushed the newpm branch from 2856d04 to 4903c07 Compare April 10, 2021 18:07

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 Apr 10, 2021

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 27, 2021

This comment has been minimized.

Sign in to view

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

nikic added 7 commits May 8, 2021 10:58

Make -Z new-llvm-pass-manager an Option<bool>

0318883

To allow it to have an LLVM version dependent default.

Explicitly register instrprof pass

5ecbe7f

Don't use "passes" for this purpose, explicitly insert it into the correct place in the pipeline instead.

Explicitly register GCOV profiling pass as well

db140de

Drop -opt-bisect-limit=0 flag from test

7c4989a

This causes an assertion failure under NewPM, because it also ends up disabling the NameAnonGlobals pass. Instead pass -Copt-level=0 to disable optimizations. If that should be insufficient, we can use -C no-prepopulate-passes.

Support -C passes in NewPM

c2b15a6

And report an error if parsing the additional pass pipeline fails. Threading through the error accounts for most of the changes here.

Update LLVM submodule

1b928ff

This updates the LLVM submodule with recent LLVM 12.x fixes. In particular, it resolves an assertion failure when targeting AArch64 at O0.

nikic force-pushed the newpm branch from fdacbd2 to 1b928ff Compare May 8, 2021 09:02

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 May 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 9, 2021

bors merged commit 7a2f446 into rust-lang:master May 9, 2021

rustbot added this to the 1.54.0 milestone May 9, 2021

This was referenced May 9, 2021

Add asm!() support for PowerPC #84732

Merged

Add .clang-format and run clang-format -i *.cpp #84889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for NewPM #83894

Improve support for NewPM #83894

nikic commented Apr 5, 2021 •

edited

Loading

rust-highfive commented Apr 5, 2021

nikic commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 5, 2021

Mark-Simulacrum commented Apr 5, 2021

Mark-Simulacrum commented Apr 5, 2021

the8472 commented Apr 5, 2021

estebank commented Apr 8, 2021

nagisa commented Apr 8, 2021

nagisa left a comment

nikic commented Apr 10, 2021

nagisa Apr 10, 2021

nagisa commented Apr 10, 2021

nikic commented Apr 10, 2021

bors commented Apr 10, 2021

bors commented Apr 10, 2021

cuviper commented Apr 28, 2021

bors commented Apr 28, 2021

This comment has been minimized.

bors commented Apr 28, 2021

nikic commented May 7, 2021

nikic commented May 8, 2021

nagisa commented May 9, 2021

bors commented May 9, 2021

bors commented May 9, 2021

bors commented May 9, 2021

nikic commented Aug 21, 2021

		);
		result.into_result().map_err(\|()\| llvm_err(diag_handler, "Failed to run LLVM passes"))

Improve support for NewPM #83894

Improve support for NewPM #83894

Conversation

nikic commented Apr 5, 2021 • edited Loading

rust-highfive commented Apr 5, 2021

nikic commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 5, 2021

Mark-Simulacrum commented Apr 5, 2021

Mark-Simulacrum commented Apr 5, 2021

the8472 commented Apr 5, 2021

estebank commented Apr 8, 2021

nagisa commented Apr 8, 2021

nagisa left a comment

Choose a reason for hiding this comment

nikic commented Apr 10, 2021

nagisa Apr 10, 2021

Choose a reason for hiding this comment

nagisa commented Apr 10, 2021

nikic commented Apr 10, 2021

bors commented Apr 10, 2021

bors commented Apr 10, 2021

cuviper commented Apr 28, 2021

bors commented Apr 28, 2021

This comment has been minimized.

bors commented Apr 28, 2021

nikic commented May 7, 2021

nikic commented May 8, 2021

nagisa commented May 9, 2021

bors commented May 9, 2021

bors commented May 9, 2021

bors commented May 9, 2021

nikic commented Aug 21, 2021

nikic commented Apr 5, 2021 •

edited

Loading