Fix the unsoundness in the early_otherwise_branch mir opt pass
#91840
Conversation
rust-highfive
added
the
S-waiting-on-review
| // - In `BB9`: Not possible since control flow might have reached `BB9` via the | ||
| // `otherwise` branch in `BBC, BBD` in the input to our transformation, which would | ||
| // have invalidated the data when computing `discriminant(P)` |
There was a problem hiding this comment.
I realize now that depending on the MIR semantics, this may be incorrect. In particular, we can imagine BB9 doing something like this:
if Q == otherwise {
use reference invalidated by P
}
This would have been correct before this pass, since P is only computed if Q does not take the otherwise branch; it's obviously no longer correct after the pass. That being said, this kind of a pattern can't ever pass borrowck (since borrowck will not consider that the conditions in the branch in BB1 and BB9 are the same). Is this a legal transformation for other optimizations to make? Or must the validity of all references (whenever they're used) in the MIR always follow only from the control flow? (hopefully I explained myself accurately there)
There was a problem hiding this comment.
Great explanation, thanks. Unfortunately I don't have a good answer. MIR semantics are very underdocumented. As far as I know there is no opt that could cause such a pattern right now, but I'm wondering if we could write it with raw pointers and intrinsics
camelid
added
the
T-compiler
|
Will review some time this week @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 396d8c60dbd6cb21d2176ac43f9f542c37fc404b with merge 236e8f85e050e93c72957ee20322d47010a24084... |
|
☀️ Try build successful - checks-actions |
|
Queued 236e8f85e050e93c72957ee20322d47010a24084 with parent f7fd79a, future comparison URL. |
|
Finished benchmarking commit (236e8f85e050e93c72957ee20322d47010a24084): comparison url. Summary: This change led to very large relevant regressions 😿 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
|
After asking on zulip that particular one might be noise, but the rest still don't look like they're getting us anything. Will take a look later |
|
This is causing issues with incremental and there are other things I want to look at too before this gets merged. I can close and re-open instead if that's preferable |
This optimization pass previously made excessive assumptions as to the nature of the blocks being optimized. We remove those assumptions and make sure to rigorously justify all changes that are made to the MIR. Details can be found in the file.
JakobDegen
force-pushed
the
fix_early_otherwise
branch
from
396d8c6
to
c0c13b7
Compare
|
I've dropped the MIR-opt-level requirement to 2 so that this actually runs. Also fixed an ICE and made a couple of other adjustments. I think things should be ready for review. I also did local perf runs of this and it was a flat zero across the board, so I'm not expecting this to bring many benefits. Having the opt be sound should be a good step forward regardless though. |
|
Realize I forgot to comment on this: There had been a suggestion on the original version of this pass to do a |
Indeed, I just wanted to make sure we don't regress anything. I also agree with the "spurious noise" assessment on the one regression |
|
@oli-obk will you get a chance to review this, or is there someone else I should ask? If there's no review capacity right now, I can close this until there is if that's preferable |
|
Sorry, didn't get to this properly before Christmas. I'll get back to reviewing complex things like this mid January |
|
triage: friendly ping @oli-obk :) |
|
Hmm I reviewed this before but obviously never wrote any conclusions. Well, third time's the charm. The opt is really well documented and structured and I feel confident it actually catches all the cases where it shouldn't run. @bors r+ rollup=never |
|
📌 Commit c0c13b7 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a7f3757): comparison url. Summary: This benchmark run shows 99 relevant improvements 🎉 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Closes #78496 .
This change is a significant rewrite of much of the pass. Exactly what it does is documented in the source file (with ascii art!), and all the changes that are made to the MIR that are not trivially sound are carefully documented. That being said, this is my first time touching MIR, so there are probably some invariants I did not know about that I broke.
This version of the optimization is also somewhat more flexible than the original; for example, we do not care how or where the value on which the parent is switching is computed. There is no requirement that any types be the same. This could be made even more flexible in the future by allowing a wider range of statements in the bodies of
BBC, BBD(as long as they are all the same of course). This should be a good first step though.Probably needs a perf run.
r? @oli-obk who reviewed things the last time this was touched