Add a MIR pass manager #77665

ecstatic-morse · 2020-10-07T19:08:32Z

A minor refactoring to transform/mod.rs. It moves MIR phase handling and optimization level/soundness checking into a single module. By using an RAII-style PassManager, we can interleave calls to run_passes with various lints/error-checking. This also adds a flag, -Zmir-opt-skip-pass, which allows skipping MIR optimization passes by name.

I'm not sure this is exactly the direction we want to go, but I really want some form of the declarative OptLevel in this PR.

cc #72515 @rust-lang/wg-mir-opt

r? @wesleywiser

tmiasko · 2020-10-07T22:12:53Z

compiler/rustc_mir/src/transform/mod.rs

-    if mir_phase == MirPhase::Optimization {
-        validate::Validator { when: format!("end of phase {:?}", mir_phase), mir_phase }
-            .run_pass(tcx, body);
-    }


Currently we are always running validation at least once and -Zvalidate-mir enables extra validation after each pass. I mentioning this since it is unclear if changing this was intentional.

Good catch! This was accident on my part. Besides disabling unsound optimizations below MIR opt-level 2 and only running cleanup passes if we ran the preceding pass in the pipeline (both up for debate), this PR shouldn't have any functional changes.

bors · 2020-10-08T02:08:51Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

src/test/mir-opt/issue-73223.rs

src/test/mir-opt/simplify-arm-identity.rs

src/test/mir-opt/issue_73223.main.PreCodegen.64bit.diff

oli-obk · 2020-10-08T05:45:02Z

compiler/rustc_mir/src/transform/pass.rs

+    /// A `Cleanup` pass is skipped if the pass immediately preceding it is skipped.
+    //
+    // FIXME: Maybe we want to run the cleanup unless *all* passes between it and the last cleanup
+    // were skipped?


This seems a bit fragile to me. Several optimizations nowadays don't need a cleanup after them, because they just invoke the appropriate cleanup method within themselves. Maybe we should migrate to that scheme in general and get rid of all the cleanup passes? That could make diffs harder to read though, so not sure. It does make diffs harder to read in some of the passes with integrated cleanup

Yeah, I don't really have a good solution for this. The status quo is equivalent to giving each instance of a cleanup pass a different LEVEL. I could do this with const-generics, or I could switch from an associated const to an instance method. The latter would make the MirPass trait object-safe again, meaning I could get rid of the run_passes macro, but I really like the declarative nature of associated constants.

Thinking about it again, to preserve the current situation, only running a cleanup if the MIR wasn't modified since the last same cleanup doesn't seem too bad. Approximating "MIR wasn't modified" with "no MIR passes were run" is also ok.

The FIXME should be resolved before this PR is merged, as otherwise we may be missing cleanups that were previously triggered even if there were multiple other not-running optimizations in between.

compiler/rustc_mir/src/transform/pass.rs

compiler/rustc_mir/src/transform/dest_prop.rs

compiler/rustc_interface/src/tests.rs

bjorn3 · 2020-10-09T18:59:42Z

compiler/rustc_mir/src/transform/validate.rs

-    /// itself.
-    pub mir_phase: MirPhase,
-}
+pub fn validate_body_during_phase(


This function seems to only be used through validate_body.

I'm aware. It'll be useful later.

wesleywiser · 2020-10-09T22:00:28Z

I didn't give high level feedback with my review comments but I really like the direction of this PR in general and I'd be happy to r+ it once the review feedback is resolved!

In general, read-only passes should not be declared as a `MirPass`, since they always run and their MIR dump provides no additional information.

These don't actually test anything since whoever implemented `-Zunsound-mir-opts` forgot to set it when unsound passes were checked. They break now that the passes are skipped entirely without `-Zunsound-mir-opts` instead of just being no-ops.

bors · 2020-10-13T03:16:37Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

camelid · 2020-11-20T03:35:41Z

@ecstatic-morse Could you resolve the merge conflicts?

Dylan-DPC-zz · 2020-11-25T15:11:35Z

closing this due to inactivity

…li-obk Add a MIR pass manager (Taylor's Version) The final draft of rust-lang#91386 and rust-lang#77665. While the compile-time constraints in rust-lang#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 assigned wesleywiser Oct 7, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2020

ecstatic-morse force-pushed the mir-pass-manager branch 3 times, most recently from 203d27c to 67142ed Compare October 7, 2020 19:39

tmiasko reviewed Oct 7, 2020

View reviewed changes

oli-obk reviewed Oct 8, 2020

View reviewed changes

src/test/mir-opt/issue-73223.rs Outdated Show resolved Hide resolved

oli-obk reviewed Oct 8, 2020

View reviewed changes

src/test/mir-opt/simplify-arm-identity.rs Show resolved Hide resolved

oli-obk reviewed Oct 8, 2020

View reviewed changes

src/test/mir-opt/issue_73223.main.PreCodegen.64bit.diff Show resolved Hide resolved

oli-obk reviewed Oct 8, 2020

View reviewed changes

wesleywiser reviewed Oct 8, 2020

View reviewed changes

compiler/rustc_mir/src/transform/pass.rs Show resolved Hide resolved

wesleywiser reviewed Oct 8, 2020

View reviewed changes

compiler/rustc_mir/src/transform/dest_prop.rs Show resolved Hide resolved

wesleywiser reviewed Oct 8, 2020

View reviewed changes

compiler/rustc_interface/src/tests.rs Show resolved Hide resolved

bjorn3 reviewed Oct 9, 2020

View reviewed changes

ecstatic-morse force-pushed the mir-pass-manager branch 2 times, most recently from 1a7ed98 to 7a8729d Compare October 11, 2020 01:43

ecstatic-morse added 5 commits October 10, 2020 19:35

Define a PassManager for managing MIR passes

da4cb6c

Declare opt-level for each MIR transformation

5f98540

Validate is not a MIR transformation

5b46f91

In general, read-only passes should not be declared as a `MirPass`, since they always run and their MIR dump provides no additional information.

Add mechanism for skipping MIR passes

51da28a

ecstatic-morse force-pushed the mir-pass-manager branch from 7a8729d to d325564 Compare October 11, 2020 02:35

ecstatic-morse marked this pull request as ready for review October 11, 2020 02:36

camelid added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2020

tmiasko mentioned this pull request Nov 10, 2020

Add flags customizing behaviour of MIR inlining #78873

Merged

Dylan-DPC-zz closed this Nov 25, 2020

oli-obk mentioned this pull request Dec 15, 2020

Tracking Issue for Non-Lexical Constant Propagation #80038

Closed

4 tasks

spastorino mentioned this pull request Mar 4, 2021

Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default #82736

Merged

oli-obk mentioned this pull request Oct 28, 2021

Replace some operators in libcore with their short-circuiting equivalents #90346

Merged

This was referenced Nov 30, 2021

Add a MIR pass manager, take 2 #91386

Closed

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a MIR pass manager #77665

Add a MIR pass manager #77665

ecstatic-morse commented Oct 7, 2020 •

edited

Loading

tmiasko Oct 7, 2020

ecstatic-morse Oct 7, 2020

bors commented Oct 8, 2020

oli-obk Oct 8, 2020

ecstatic-morse Oct 10, 2020

oli-obk Oct 12, 2020

bjorn3 Oct 9, 2020

ecstatic-morse Oct 10, 2020

wesleywiser commented Oct 9, 2020

bors commented Oct 13, 2020

camelid commented Nov 20, 2020

Dylan-DPC-zz commented Nov 25, 2020

Add a MIR pass manager #77665

Add a MIR pass manager #77665

Conversation

ecstatic-morse commented Oct 7, 2020 • edited Loading

tmiasko Oct 7, 2020

Choose a reason for hiding this comment

ecstatic-morse Oct 7, 2020

Choose a reason for hiding this comment

bors commented Oct 8, 2020

oli-obk Oct 8, 2020

Choose a reason for hiding this comment

ecstatic-morse Oct 10, 2020

Choose a reason for hiding this comment

oli-obk Oct 12, 2020

Choose a reason for hiding this comment

bjorn3 Oct 9, 2020

Choose a reason for hiding this comment

ecstatic-morse Oct 10, 2020

Choose a reason for hiding this comment

wesleywiser commented Oct 9, 2020

bors commented Oct 13, 2020

camelid commented Nov 20, 2020

Dylan-DPC-zz commented Nov 25, 2020

ecstatic-morse commented Oct 7, 2020 •

edited

Loading