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

Add a MIR pass manager, take 2 #91386

Closed

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Nov 30, 2021

A more powerful version of #77665.

The goal is to allow fine-grained ordering constraints between passes and to make violating those constraints a compile-time error (and partly to motivate #90488 😏). The compile-time part would be very hard when enforcing constraints across queries, so we continue to rely on MirPhase for things like "optimizations run after drop elaboration". However, it works okay within a single invocation of run_passes (e.g. all the optimizations), where adding a few constraints to each pass is less of a pain than adding a bunch of variants to MirPhase. Since constraints are declared explicitly, we can do cool stuff like check that they hold for all possible opt-levels, -Z flags, etc.

For now it's just a proof of concept, and a silly one at that. I don't know that we ultimately want to go in this direction, but it was pretty fun. It turns out writing nested for loops in a const-context leads to some pretty ugly code. @oli-obk might be into it at least 😄. I only translated one small section of MIR passes so that people could get a feel for how it would look. We probably will want a different interface; the batch of associated constants is pretty unpleasant to look at IMO.

All the interesting stuff is in the last few commits. The rest are refactorings or const-eval stuff to get everything working. Ideally, each pass would declare which cleanup passes it needs, but for that I'll need to move all the MIR passes back into rustc_mir_transform.

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2021
@ecstatic-morse
Copy link
Contributor Author

r? @ghost

@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)
    Checking rustc_mir_transform v0.0.0 (/checkout/compiler/rustc_mir_transform)
error: unused import: `Lint`
  --> compiler/rustc_mir_transform/src/lib.rs:41:26
   |
41 | use self::pass_manager::{Lint, MirPassC};
   |
   |
   = note: `-D unused-imports` implied by `-D warnings`
error: could not compile `rustc_mir_transform` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed
Build completed unsuccessfully in 0:02:51

}

tcx.sess.mir_opt_level() >= 3
tcx.sess.opts.debugging_opts.inline_mir && tcx.sess.mir_opt_level() >= 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the condition. Before it would always run if -Zinline-mir=yes is used, never if -Zinline-mir=no is used and only for mir opt level >= 3 if -Zinline-mir is not specified. After it will only run when -Zinline-mir=yes and mir opt level >= 3 is combined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I'm sorry. I understand the Option<bool> now.

@bors
Copy link
Contributor

bors commented Dec 3, 2021

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

@ecstatic-morse
Copy link
Contributor Author

Closing in favor of the more minimal #91475. I'd like to explore some of these ideas in the future, though.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants