-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Do not emit alloca for ZST locals with multiple assignments #86166
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit e8b2d46ac74d3e072631d607a868935393b47e66 with merge 004afd4df22776fea09704d480c8e7b8b1c10a1e... |
☀️ Try build successful - checks-actions |
Queued 004afd4df22776fea09704d480c8e7b8b1c10a1e with parent c4b5406, future comparison URL. |
r? @nagisa |
Finished benchmarking try commit (004afd4df22776fea09704d480c8e7b8b1c10a1e): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
When rebuilding the standard library with `-Zbuild-std` this reduces the number of locals that require an allocation from 62315 to 61767.
e8b2d46
to
40c9aae
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 40c9aae with merge 6f83711458d087a929d9bedb62c37342e448eb97... |
☀️ Try build successful - checks-actions |
Queued 6f83711458d087a929d9bedb62c37342e448eb97 with parent 0279cb1, future comparison URL. |
Finished benchmarking try commit (6f83711458d087a929d9bedb62c37342e448eb97): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
This seems pretty low value to me. We already don't emit any load/store operations on the ZST allocas (AFAIR), and so these allocas are trivially removable by LLVM. The perf data seems similarly mixed. But I do find the new code nicer to read, so @bors r+ |
📌 Commit 40c9aae has been approved by |
☀️ Test successful - checks-actions |
This extends 35566bf to additionally stop emitting unnecessary allocas for zero sized locals that are assigned multiple times.
When rebuilding the standard library with
-Zbuild-std
this reduces the number of locals that require an allocation from 62315 to 61767.