Add support for MIR opt unit tests via a new -Z flag

[JakobDegen]

Proposal

Motivation

MIR opt tests currently suffer from being somewhat brittle. Each test indirectly includes the output of every pass that runs before the pass that test is targeting. Changing pass order or the behavior of some passes can hence break the tests for unrelated subsequent passes. Furthermore, many (most?) of the MIR tests are intended to be "unit tests" - that is, they intend to verify that a particular pass behaves in a particular way on particular input.

In some cases, when unrelated passes change these unit tests can be broken in trivial ways that can just be blessed. This is not a huge deal, but it increases the size of the diff and requires the contributor to evaluate the change to a test unrelated to the code they are writing. Other times, the test changes in a non-trivial way - most commonly, this is in the form of the input to the pass being tested having changed and the test no longer demonstrating what it was supposed to. The contributor then has to go and change the test in such a way as to reproduce the old behavior. Depending on the nature of the conflict, this can be difficult to do either at all or in a clean way. Of course, even if successful, this once more causes an increase in the size of the diff, increased review burden, etc.

Proposal

The proposal consists of two parts:

1. Add support for a new, permanently unstable -Zmir-enable-passes=+DestProp,-InstCombine flag. MIR passes currently specify their own behavior for when to be enabled; this flag would override that behavior. This also means it ignores -Zunsound-mir-opts and -Zmir-opt-level.

2. Add a new header directive to MIR opt tests that looks something like this: // unit-test MyPassName. This would cause compiletest to emit -Zmir-opt-level=0, -Zmir-enable-passes=+MyPassName, and the remaining flags that it currently emits. The effect would be that the only optimization pass that runs is the named one. Non-optimization passes would of course still run.

   These tests will continue to use // EMIT-MIR annotations for their output. However, the meaningful set of annotations that could be used would basically be reduced to MyPassName.diff and MyPassName.after, since no other passes run.

   If a test requires other passes to run as well then such passes could additionally be specified with -Zmir-enable-passes flags via the compiler flags header directive. At some point in the future we may want to consider enabling some small set of passes by default, if it becomes common for other passes to depend on the MIR having been brought into some canonicalized state - I do not propose adding such a thing now.

Drawbacks and Alternatives

1. This adds an additional requirement to the MIR pass manager that may impact our ability to refactor it in the future. If the logic for enabling/disabling passes becomes more complicated, there may be no simple way to implement the -Zmir-enable-passes flag. However, I believe that a concept of "MIR opt unit tests" will be necessary regardless of how the pass manager is implemented. I do not think that any pass manager which prevents such tests from existing without supplying an alternative would be a good idea.
2. An MIR parser - either as in Implement a MIR parser for testing purposes rust#91669 or via some of the stable MIR work - might reduce the need for such a test mode. However, I do not think it would eliminate it - possibly it would even increase the need for such a test mode, due to "I wrote this MIR in the test file but another pass mangled it before it got to me."

Mentors or Reviewers

I am planning to implement these changes myself and do not expect to require significant mentoring to do so.

@nagisa has said that they would be willing to review. I also do not expect this to require particularly complex implementation though, so the review burden should not be too great.

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.

[rustbot]

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

[nagisa]

@rustbot second

[apiraino]

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