Implement MIR opt unit tests #96090
Implement MIR opt unit tests #96090
Conversation
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
| let name = pass.name(); | ||
|
|
||
| if let Some((_, polarity)) = overridden_passes.iter().rev().find(|(s, _)| s == &*name) { |
There was a problem hiding this comment.
I wonder if we should use a StableMap for overridden_passes. This looks awfully like a n^2 situation, although it'll probably never end up with enough values to make it matter much? One other reason to use a StableMap would be to reduce rebuilds from this option being changed in a ways that don't actually matter (e.g. -Zmir-enable-passes=+InstCombine,-InstCombine has no functional difference compoared to -Zmir-enable-passes=-InstCombine)
There was a problem hiding this comment.
I had considered this but actually intentionally avoided it - my reasoning was that the most important scenario for perf is the one where overridden_passes is empty (since that's what essentially every user will be using), and Vec is a data structure that I can be very certain is very fast in that case.
| @@ -189,6 +191,7 @@ mod directives { | |||
| pub const STDERR_PER_BITWIDTH: &'static str = "stderr-per-bitwidth"; | |||
| pub const INCREMENTAL: &'static str = "incremental"; | |||
| pub const KNOWN_BUG: &'static str = "known-bug"; | |||
| pub const MIR_UNIT_TEST: &'static str = "unit-test"; | |||
There was a problem hiding this comment.
Hm, adding this seems useful, but I'm torn over this effectively duplicating compile-flags option. On one hand I can see how compile-flags: -Zmir-opt-level=0 -Zmir-enable-passes=+ThePass can get boring quickly, on another it never really was a huge problem with -Cno-prepopulate-passes in LLVM tests so far.
I think there's might be some value in having a somewhat uniform mechanism to achieve similar things across tests exercising different parts of the codebase.
There was a problem hiding this comment.
Yeah, this is valid. The one thing that I'm keeping in the back of my mind as motivation for this is the potential to turn on some canonicalization passes by default in the future. At that point we will certainly want this header, since we don't want tests getting out of sync.
Besides that, I don't have an opinion either way. It totally might make sense to delay this until we have a need to turn on passes by default.
|
@bors r+ |
|
📌 Commit 4534188 has been approved by |
bors
added
S-waiting-on-bors
…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#95395 (Better error message for `_` in function signature in `impl Trait for Ty`) - rust-lang#96090 (Implement MIR opt unit tests) - rust-lang#96107 ([test] Add test cases for untested functions for VecDeque) - rust-lang#96212 (Use revisions instead of nll compare mode for `/regions/` ui tests) - rust-lang#96215 (Drop support for legacy PM with LLVM 15) - rust-lang#96366 (bootstrap: Remove dead code in rustdoc shim) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This implements rust-lang/compiler-team#502 .
There's not much to say here, this implementation does everything as proposed. I also added the flag to a bunch of existing tests (mostly those to which I could add it without causing huge diffs due to changes in line numbers). Summarizing the changes to test outputs:
MirPatchis created, it adds a cleanup block to the body if it did not exist already. If this block is unused (as is usually the case), it usually gets removed soon after by some pass callingSimplifyCFGfor unrelated reasons (in many cases this cycle happens quite a few times for a single body). We now runSimplifyCFGless often, so those blocks end up in some of our outputs. I looked at changingMirPatchto not do this, but that seemed too complicated for this PR. I may still do that in a follow-up.InstCombinetest had set-C opt-level=0in its flags and so there were no storage markers. I don't really see a good motivation for doing this, so bringing it back in line with what everything else does seems correct.EarlyOtherwiseBranchtests hadUnreachableProprunning on it. Preventing that kind of thing is the goal of this feature, so this seems fine.For the remaining tests for which this feature might be useful, we can gradually migrate them as opportunities present themselves.
In terms of documentation, I plan on submitting a PR to the rustc dev guide in the near future documenting this and other recent changes to MIR. If there's any other places to update, do let me know
r? @nagisa