-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Fix variable debuginfo being optimized away at mir-opt-level=2
#103657
Fix variable debuginfo being optimized away at mir-opt-level=2
#103657
Conversation
The `DeadStoreElmination` pass was removing the uses of these locals because the are never used in the MIR body except for debuginfo. I've updated the pass to consider locals referenced by debuginfo as always alive unless a higher MIR opt level is requested by the user.
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
fec7a06
to
daf2968
Compare
According to rust-lang/compiler-team#319 , mir opt level 2 is not required to maintain debuggability. Has this changed? |
This comment has been minimized.
This comment has been minimized.
It changed in #82736 which was merged after that policy. I looked briefly but don't see our policy actually documented anywhere other than that closed MCP. I might take a stab at introducing an enum for |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit daf2968 with merge ba1d739bab63e852f8d36184b10d102e02ac3113... |
I'm incredibly confused. When I asked about the policy on Zulip no one could seem to agree what it was, and now this PR is proposing another option. Certainly if this is right then lots of my commits to the repo have been wrong (although I'm not the only one). I also have some thoughts about a mir opt level policy that doesn't allow us to regress debuginfo on any opt level that users will realistically be turning on. It seems like we need a new MCP on this topic, given that the previous one does not seem to have resolved this topic |
That's a fair concern! I agree, the current situation is very confusing, probably because we never really documented it anywhere and then it was later modified without a clear explanation of the new level being introduced. Sorting this out via an MCP and then actually documenting it somewhere seems like a reasonable next step. At the same time, it seems like a bug to me that disabling MIR optimizations for code like that of the test gives you both better debuginfo and doesn't hurt code quality either (LLVM is able to perform this optimization just as well as we can but they do it while preserving debuginfo). If there isn't much of a compile time impact from this change, I think we should merge it. |
Sure, I don't feel particularly strongly about this PR specifically (although that may change in the future). That being said, do we know what is needed in order for us to fix debuginfo in the same way LLVM does? |
Yet another option (sorry), would be to tie the presence of debuginfo in MIR to |
I think I put most of the infrastructure in place to support this in #73210 but it wasn't enabled because it wasn't working correctly on Windows. At the time, I knew nothing about Windows debuginfo so perhaps now I can resolve whatever that issue was. 😄 |
I've been thinking for a while that optimization "levels" might be a pretty bad solution to the problem they're trying to solve. Maybe this is a good opportunity to revisit this problem from the beginning, start by figuring out what the use cases are that we're trying to support, and design from there |
☀️ Try build successful - checks-actions |
Queued ba1d739bab63e852f8d36184b10d102e02ac3113 with parent 0da281b, future comparison URL. |
Finished benchmarking commit (ba1d739bab63e852f8d36184b10d102e02ac3113): comparison URL. Overall result: ❌ regressions - 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.
Footnotes |
Just trying to see what we need to do to recover the throughput (if possible). @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 51d5932 with merge 5552e8f2ea34ebec9325fbba2b896872124d77f4... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
Queued 5552e8f2ea34ebec9325fbba2b896872124d77f4 with parent 5237c4d, future comparison URL. |
Finished benchmarking commit (5552e8f2ea34ebec9325fbba2b896872124d77f4): comparison URL. Overall result: ❌ regressions - 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
Turn on ConstDebugInfo pass. Split from rust-lang#103657 Moving those constant into debuginfo allows to shrink the number of locals and the actual size of the MIR body.
@wesleywiser Can you please post your status on this PR? It has sat idle for months. |
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. @rustbot label: +S-inactive |
The
DeadStoreElmination
pass was removing the uses of these localsbecause the are never used in the MIR body except for debuginfo. I've
updated the pass to consider locals referenced by debuginfo as always
alive unless a higher MIR opt level is requested by the user.
Fixes #103655
r? @ghost