-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Only enable ConstProp at mir-opt-level >= 2. #109900
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit e5fa186b67f621a6d2a57e358577d3ee768022f7 with merge 32a9fa12c38e5dfd03b2d1bbf3e585b088249631... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
I'm pretty sure this won't break this mir-opt test, but it should: rust/tests/mir-opt/div_overflow.rs Line 1 in cf7ada2
I think it won't break it because the mir-opt tests all override |
Finished benchmarking commit (32a9fa12c38e5dfd03b2d1bbf3e585b088249631): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking 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 may lead 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
So we lose some perf for
This makes me wonder if the const prop PR is useful at all (so also in release mode) except to provide simpler MIR for other MIR opts? |
This comment has been minimized.
This comment has been minimized.
hmm... do you think this is an issue? I think the rest of the tests (ui tests) should suffice for testing the debug/release defaults. |
Yes, that is an issue. That mir-opt test is supposed to check that we don't emit a specific unreachable panic branch in debug mode, and after this PR we will again emit the undesirable panic. That test never tested what it was supposed to. It should have been a codegen test. If you are deciding in this PR to regress our codegen in debug builds, we should reopen that issue. Maybe someone can find another way to get the desired codegen. |
788547e
to
1d1bee2
Compare
the relevant issue is #72751 While this PR regresses that issue, the issue had no motivation explicitly mentioned. As long as we were able to trivially support it, that seems like a no-brainer, but I don't see a strong reason for actually avoiding emitting the panic path in debug builds. If there is such a reason, we should of course do something about it, but I found none in the PRs or issues linked to or from #72751 |
☔ The latest upstream changes (presumably #102906) made this pull request unmergeable. Please resolve the merge conflicts. |
1d1bee2
to
4a5c148
Compare
@bors r+ |
📌 Commit 4a5c14877c0d8cdca2b3d0454616bac63b2efbf7 has been approved by It is now in the queue for this repository. |
This comment has been minimized.
This comment has been minimized.
That failure is an existing bug in codegen which was hidden by const-prop 😭. Filed #110128 while I investigate a fix. |
Blocked on #110197 |
4a5c148
to
483525e
Compare
@bors r=oli-obk |
☀️ Test successful - checks-actions |
Finished benchmarking commit (67e273b): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
That pass is not responsible for lints any more, so we can restrict it to optimized builds.
This reduces the amount of duplicated const-eval messages.