Add a MIR pass manager (Taylor's Version) #91475

ecstatic-morse · 2021-12-02T23:11:10Z

The final draft of #91386 and #77665.

While the compile-time constraints in #91386 are cool, I decided on a more minimal approach for now. I want to explore phase constraints and maybe relative-ordering constraints in the future, though. This should preserve existing behavior exactly (please let me know if it doesn't) while making the following changes to the way we organize things today:

Each MirPhase now corresponds to a single MIR pass. run_passes is not responsible for listing the correct MIR phase.
run_passes no longer silently skips passes if the declared MIR phase is greater than or equal to the body's. This has bitten me multiple times. If you want this behavior, you can always branch on body.phase yourself.
If your pass is solely to emit errors, you can use the MirLint interface instead, which gets a shared reference to Body instead of a mutable one. By differentiating the two, I hope to make it clearer in the short term where lints belong in the pipeline. In the long term perhaps we could enforce this at compile-time?
MIR is no longer dumped for passes that aren't enabled, or for lints.

I tried to check that -Zvalidate still works correctly, since the MIR phase is now updated as soon as the associated pass is done, instead of at the end of all the passes in run_passes. However, it looks like -Zvalidate is broken with current nightlies anyways 😢 (it spits out a bunch of errors).

cc @oli-obk @wesleywiser

r? rust-lang/wg-mir-opt

rust-highfive · 2021-12-02T23:11:13Z

r? @jackh726

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

compiler/rustc_mir_transform/src/lower_slice_len.rs

Looks like Generator drop shims already have `post_borrowck_cleanup` run on them. That's a bit surprising, since it means they're getting const- and maybe borrow-checked? This merits further investigation, but for now just preserve the status quo.

The noopt test never actually ran the pass

oli-obk · 2021-12-03T07:21:04Z

r? @oli-obk

oli-obk

This is really nice. It will make further changes so much easier to impl and reason about.

Also thx for the clean commits, reviewing was real easy.

r=me with ci passing, perf clean and the comment removed

oli-obk · 2021-12-03T07:41:42Z

compiler/rustc_mir_transform/src/lib.rs

+            // FIXME(#70073): This pass is responsible for both optimization as well as some lints.
+            &const_prop::ConstProp,
+            //
+            // FIXME: The old pass manager ran this only at mir-opt-level >= 1, but


While const prop runs unconditionally, it doesn't change the MIR unless the opt level is 1 or above. So we indeed don't need to simplify unconditionally

oli-obk · 2021-12-03T07:45:50Z

@bors try @rust-timer queue

rust-timer · 2021-12-03T07:45:51Z

Awaiting bors try build completion.

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

bors · 2021-12-03T07:46:02Z

⌛ Trying commit 7bc5eca with merge 0d6119ad550f626d8dac7253fa6d5897ec3d3667...

bors · 2021-12-03T09:13:48Z

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

rust-log-analyzer · 2021-12-04T15:06:36Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2021-12-04T17:19:36Z

⌛ Testing commit 7bc5eca with merge 23fd1e950a1e0b895d1f1e5e0cc3f444c7ca21ce...

bors · 2021-12-04T17:22:22Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-12-04T17:31:14Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

matthiaskrgr · 2021-12-04T17:37:55Z

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0curl: (6) Could not resolve host: ci-mirrors.rust-lang.org
@bors retry

bors · 2021-12-05T03:41:22Z

⌛ Testing commit 7bc5eca with merge bdaa901...

bors · 2021-12-05T06:46:52Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing bdaa901 to master...

rust-timer · 2021-12-05T08:18:10Z

Finished benchmarking commit (bdaa901): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

Moderate improvement in instruction counts (up to -0.8% on full builds of stm32f4)

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

@rustbot label: -perf-regression

…anup, r=oli-obk Address some FIXMEs left over from rust-lang#91475 This shouldn't change behavior, only clarify what we're currently doing. I filed rust-lang#91576 to see if the treatment of generator drop shims is intentional. cc rust-lang#91475

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#83744 (Deprecate crate_type and crate_name nested inside #![cfg_attr]) - rust-lang#90550 (Update certificates in some Ubuntu 16 images.) - rust-lang#91272 (Print a suggestion when comparing references to primitive types in `const fn`) - rust-lang#91467 (Emphasise that an OsStr[ing] is not necessarily a platform string) - rust-lang#91531 (Do not add `;` to expected tokens list when it's wrong) - rust-lang#91577 (Address some FIXMEs left over from rust-lang#91475) - rust-lang#91638 (Remove `in_band_lifetimes` from `rustc_mir_transform`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Dec 2, 2021

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

ecstatic-morse commented Dec 2, 2021

View reviewed changes

compiler/rustc_mir_transform/src/lower_slice_len.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the mir-pass-manager3 branch from ec8cb0b to 92e92db Compare December 3, 2021 00:21

ecstatic-morse changed the title ~~Add a MIR pass manager (Taylor's version)~~ Add a MIR pass manager (Taylor's Version) Dec 3, 2021

ecstatic-morse added 2 commits December 2, 2021 17:12

Move mir_opt_level getter into Options

386b1c5

Move instrument coverage config getters to Options

a0de634

ecstatic-morse force-pushed the mir-pass-manager3 branch 2 times, most recently from cefefc4 to 37ac4ad Compare December 3, 2021 01:29

ecstatic-morse added 7 commits December 2, 2021 17:31

Implement a pass manager

c1a501b

Update passes with new interface

fd18b45

Add pass for simple phase change

fca642c

Use new MIR pass manager

71dd542

Don't "simplify" during optimizations if optimizations are disabled

dc5feeb

EarlyOtherwiseBranch requires -Zunsound-mir-opts

7bc5eca

The noopt test never actually ran the pass

ecstatic-morse force-pushed the mir-pass-manager3 branch from 37ac4ad to 7bc5eca Compare December 3, 2021 01:31

ecstatic-morse mentioned this pull request Dec 3, 2021

Add a MIR pass manager, take 2 #91386

Closed

rust-highfive assigned oli-obk and unassigned jackh726 Dec 3, 2021

oli-obk approved these changes Dec 3, 2021

View reviewed changes

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

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 4, 2021

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 Dec 4, 2021

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 4, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 5, 2021

bors merged commit bdaa901 into rust-lang:master Dec 5, 2021

rustbot added this to the 1.59.0 milestone Dec 5, 2021

This was referenced Dec 5, 2021

[WIP] Test performance of running MIR inliner on inline(always) function calls #82280

Closed

Mir-Opt for copying enums with large discrepancies #85158

Merged

Allow reifying intrinsics to fn pointers. #86699

Closed

This was referenced Dec 6, 2021

Which MIR passes should run on generator drop shims? #91576

Open

Address some FIXMEs left over from #91475 #91577

Merged

matthiaskrgr mentioned this pull request Dec 8, 2021

Rollup of 14 pull requests #91650

Closed

matthiaskrgr mentioned this pull request Dec 8, 2021

Rollup of 7 pull requests #91656

Merged

hellow554 mentioned this pull request Jan 18, 2022

rustc commits SIGSEGV: invalid memory reference) #93022

Closed

camsteffen mentioned this pull request Jan 25, 2022

Clarify stages of MIR pipeline, and make MIR lints consistent #72515

Open

Add a MIR pass manager (Taylor's Version) #91475

Add a MIR pass manager (Taylor's Version) #91475

Uh oh!

Conversation

ecstatic-morse commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 2, 2021

Uh oh!

Uh oh!

This comment has been minimized.

oli-obk commented Dec 3, 2021

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

oli-obk Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Dec 3, 2021

Uh oh!

rust-timer commented Dec 3, 2021

Uh oh!

bors commented Dec 3, 2021

Uh oh!

bors commented Dec 3, 2021

Uh oh!

rust-log-analyzer commented Dec 4, 2021

Uh oh!

bors commented Dec 4, 2021

Uh oh!

bors commented Dec 4, 2021

Uh oh!

rust-log-analyzer commented Dec 4, 2021

Uh oh!

matthiaskrgr commented Dec 4, 2021

Uh oh!

bors commented Dec 5, 2021

Uh oh!

bors commented Dec 5, 2021

Uh oh!

rust-timer commented Dec 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ecstatic-morse commented Dec 2, 2021 •

edited

Loading