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

Revise how MIR variants are distinguished #522

Closed
1 of 3 tasks
JakobDegen opened this issue Jun 27, 2022 · 4 comments
Closed
1 of 3 tasks

Revise how MIR variants are distinguished #522

JakobDegen opened this issue Jun 27, 2022 · 4 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@JakobDegen
Copy link

Background

In rustc today, MIR goes through a number of transformations between being built and being codegened. Mistakes in these transformations tend to manifest as miscompilations, and so their correctness is exceedingly important. Particularly the various MIR optimization passes are especially easy to get wrong and the history of soundness bugs reflect this. In order to begin improving the story here, we need to clarify MIR as an IR and improve the surrounding compiler infrastructure.

Recently, rust-lang/rust#95320 took a significant step in this direction by documenting much of the semantics of MIR. However, it also revealed further weaknesses, particularly around the many variants of MIR. Many passes change the "rules" for the IR in some way, and the large number of rule changes make it difficult to know what the rules at a particular point are and if a particular assumption or transformation is sound. These changes can be divided into two groups:

  1. Semantic changes. A change is a semantic change if the same MIR can have different meaning (in the sense of its operational semantics) before and after the change. Semantic changes, despite looking like standard "transformations," are actually closer to lowerings between different IRs. I will refer to the variants of MIR delimited by semantic changes as "dialects."
  2. Non-semantic changes. These are all other transformations that adjust the "rules" of MIR in some sense. They are the closest approximation of what we currently call phases, and so I will continue to refer to these as such.

Currently, all of the following MirPasses are semantic changes:

  • ElaborateDrops
  • AbortUnwindingCalls
  • AddMovesForPackedDrops
  • AddRetag
  • generator::StateTransform

One could also include const_prop_lint::ConstProp, depending on whether one considers the emission of lints a part of MIR's semantics.

Proposal

This proposal essentially says that we should agree to adopt the mental model described above and we should take a disciplined approach to adhering to it within the compiler. Specifically, this means we:

  1. Clearly identify and distinguish between all MIR dialects, restricting semantic changes to MIR to only be permitted within the lowerings between these dialects. Additionally, the number of dialects should be minimized as much as possible. The intent is for there to be only two - what I have been calling analysis and runtime MIR.
  2. Minimize the amount of code that participates in the lowerings between dialects. This means re-ordering some passes in the MIR pipeline. These changes should mostly be insignificant; however, specifically the generator transform pass will have to be moved up by quite a bit. I generally think this is a feature, not a bug.
  3. On a best effort basis, make the list of MIR phases more complete. Ideally, adding a new MIR phase should be low overhead and encouraged whenever it makes sense.
  4. Document which dialects and phases each MIR consumer expects to see. In many cases this will be fairly straightforward, but in some (eg const eval) it might not.
  5. Adjust various pieces of infrastructure in the compiler, including at least the pass manager, the definition of MirPhase, and various pieces of documentation, to reflect the proposals in the previous bullet points. We should enforce in code the parts of this that we can.

External Projects

As I mentioned above, the primary goal here is to agree on a general direction for MIR variants such that we avoid bugs and reduce complexity in the compiler. However, it may additionally be worth evaluating how this interacts with external initiatives like a-mir-formality and especially SMIR. If there are fundamental, hard-to-fix incompatibilities with rustc moving in this direction, this at least seems worth knowing.

Mentors or Reviewers

None in particular. The diffs resulting from this change are likely to end up being relatively few lines of code spread across a large-ish part of the compiler, so this hopefully won't end up being too much work for any one person.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@JakobDegen JakobDegen added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Jun 27, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jun 27, 2022
@RalfJung
Copy link
Member

Also see rust-lang/rust#86299

@oli-obk
Copy link
Contributor

oli-obk commented Jun 27, 2022

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jun 27, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 30, 2022
@apiraino
Copy link
Contributor

apiraino commented Jul 7, 2022

@rustbot label -final-comment-period +major-change-accepted

@apiraino apiraino closed this as completed Jul 7, 2022
@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jul 7, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 21, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 30, 2022
Rework definition of MIR phases to more closely reflect semantic concerns

Implements most of rust-lang/compiler-team#522 .

I tried my best to restrict this PR to the "core" parts of the MCP. In other words, this includes just enough changes to make the new definition of `MirPhase` make sense. That means there are a couple of FIXMEs lying around. Depending on what reviewers prefer, I can either fix them in this PR or send follow up PRs. There are also a couple other refactorings of the `rustc_mir_transform/src/lib.rs` file that I want to do in follow ups that I didn't leave explicit FIXMEs for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants