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

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

Open
ecstatic-morse opened this issue May 23, 2020 · 8 comments
Open

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

ecstatic-morse opened this issue May 23, 2020 · 8 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 23, 2020

The rustc_mir::transform module defines the pipeline of analysis passes and transformations that run on the MIR after it is lowered but before it is passed to codegen. However, it's not always clear whether certain invariants apply to each stage of the pipeline. For example, some terminators (FalseEdges, DropAndReplace) are removed entirely and do not appear beyond a certain point and the Return terminator, which initially appears only once in the entire MIR body may appear multiple times after the generator transform.

Additionally, it can be hard to find the right place for new error-emitting passes like the lint in #72270 or #71824. Is there a point at which the MIR is no longer meant for analysis but only for codegen? Should these be part of a specific query? Finally, the names of the mir_* queries have lost their meaning over time: mir_const can probably be removed entirely, and optimized_mir and promoted_mir should probably be renamed to mir_optimized and mir_promoted_fragments_optimized respectively.

I've listed quite a few issues above, but to resolve them I think we first need to figure out who the stakeholders are. Is there anyone who "owns" the current structure and has a clear idea of how things should look?

@ecstatic-morse ecstatic-morse added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 23, 2020
@RalfJung
Copy link
Member

the Return terminator, which initially appears only once in the entire MIR body may appear multiple times after the generator transform.

Ah I didn't know this; this should also be reflected in the docs for TerminatorKind::Return.

@RalfJung
Copy link
Member

mir_promoted_fragments_optimized

I don't think I understand what this name means. What would be the doc comment that goes with it?

@ecstatic-morse
Copy link
Contributor Author

promoted_mir doesn't do promotion (I believe that happens in mir_validated?). It steals the promoted MIR and optimizes it.

@RalfJung
Copy link
Member

Why does it need to be a separate query?

RalfJung added a commit to RalfJung/rust that referenced this issue May 30, 2020
multiple Return terminators are possible

@ecstatic-morse mentioned in rust-lang#72515 that multiple `Return` terminators are possible. Update the docs accordingly.

Cc @rust-lang/wg-mir-opt
@RalfJung RalfJung changed the title Clarify stages of MIR pipeline Clarify stages of MIR pipeline, and make MIR lints consistent Sep 7, 2020
@RalfJung
Copy link
Member

RalfJung commented Sep 7, 2020

@oli-obk added infrastructure to MIR validation to distinguish different "stages".

But we still do not have a clear place to put MIR lints, and we currently do this inconsistently. #72270 added a "MIR transform" that is just a lint; there is a lints::check call in MIR construction, and unsafety checking is its own whole thing.

@RalfJung
Copy link
Member

RalfJung commented Sep 7, 2020

Also Cc @rust-lang/wg-mir-opt for the stakeholders and code owner questions in the OP.

@camsteffen
Copy link
Contributor

Is this solved by #91475?

@RalfJung
Copy link
Member

I don't think so. We still have mir_const, mir_promoted, mir_for_ctfe, mir_drops_elaborated_and_const_checked, run_optimization_passes.

For lints, the situation seems to have improved -- we now have MirLint, that can be turned into passes. On the other hand, there also still is some mention of lints during MIR building, and check_unsafety is its own special thing and not a MIR pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants