[MIR-opt] trivial matches aren't optimized out at mir-opt-level=1 #66855
Comments
scottmcm
added
the
A-MIR
jonas-schievink
added
C-enhancement
|
You can run with -Zdump-mir locally with each -Zmir-opt-level and diff the resulting directories full of mir dumps. Then you'll see where stuff starts to diverge, which should pinpoint where we need to change something |
nagisa
added
the
requires-nightly
|
I'm surprised that this was merged without explicitly requiring |
|
@matthewjasper well... we don't have any policy about this afaik. Should we establish one? I like level 1 because it gives us good testing (as the issues have shown), but I also see that exposing new optimizations directly will likely cause problems again in the future |
|
Rather than using |
|
Or maybe use |
|
Given that bugs were found in this pass even with it requiring a Z-flag, I'm not really concerned about testing, at least for a few weeks. Having nightly not mirror stable in something with such a poor track record as newly added mir optimisations sounds terrible. |
Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1
I also added test cases to make sure the optimization can fire on all of
these cases:
```rust
fn case_1(o: Option<u8>) -> Option<u8> {
match o {
Some(u) => Some(u),
None => None,
}
}
fn case2(r: Result<u8, i32>) -> Result<u8, i32> {
match r {
Ok(u) => Ok(u),
Err(i) => Err(i),
}
}
fn case3(r: Result<u8, i32>) -> Result<u8, i32> {
let u = r?;
Ok(u)
}
```
Without MIR inlining, this still does not completely optimize away the
`?` operator because the `Try::into_result()`, `From::from()` and
`Try::from_error()` calls still exist. This does move us a bit closer to
that goal though because:
- We can now run the pass on mir-opt-level=1
- We no longer depend on the copy propagation pass running which is
unlikely to stabilize anytime soon.
Fixes #66855
Modify SimplifyArmIdentity so it can trigger on mir-opt-level=1
I also added test cases to make sure the optimization can fire on all of
these cases:
```rust
fn case_1(o: Option<u8>) -> Option<u8> {
match o {
Some(u) => Some(u),
None => None,
}
}
fn case2(r: Result<u8, i32>) -> Result<u8, i32> {
match r {
Ok(u) => Ok(u),
Err(i) => Err(i),
}
}
fn case3(r: Result<u8, i32>) -> Result<u8, i32> {
let u = r?;
Ok(u)
}
```
Without MIR inlining, this still does not completely optimize away the
`?` operator because the `Try::into_result()`, `From::from()` and
`Try::from_error()` calls still exist. This does move us a bit closer to
that goal though because:
- We can now run the pass on mir-opt-level=1
- We no longer depend on the copy propagation pass running which is
unlikely to stabilize anytime soon.
Fixes #66855
I got excited by and went to try out
SimplifyArmIdentity+SimplifyBranchSamefrom #66282, but was surprised that the first thing I attempted didn't actually get optimized away as I'd expected.Repro: https://rust.godbolt.org/z/bxFAsP
It does, however, optimize away in MIR with
-Z mir-opt-level=2.(Note that this is the simple case without
?, where there are no function calls involved so inlining should be irrelevant.)Seems like there's a gap here? cc @Centril @oli-obk @wesleywiser
Some guesses from the discord conversation where centril asked me to open this issue: the
Storage{Live|Dead}presence, extra copies because[ui]32, const prop hiding something, ...The text was updated successfully, but these errors were encountered: