-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Use preorder traversal when checking for SSA locals #85741
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
Conversation
r? @davidtwco (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 d321c04b7ba590e2129c0d2414c4749f92694ee5 with merge 4efccbc84ee38e5ba31393f12563d831ad3fa9ac... |
☀️ Try build successful - checks-actions |
Queued 4efccbc84ee38e5ba31393f12563d831ad3fa9ac with parent 9814e83, future comparison URL. |
Finished benchmarking try commit (4efccbc84ee38e5ba31393f12563d831ad3fa9ac): 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 |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit f3dd15dc8d8b5e19e6cf69cf3b0b4f78cde37aeb with merge fa6f2a233a21a3945e64b20d72b08739cf830e04... |
☀️ Try build successful - checks-actions |
Queued fa6f2a233a21a3945e64b20d72b08739cf830e04 with parent ce0d64e, future comparison URL. |
Finished benchmarking try commit (fa6f2a233a21a3945e64b20d72b08739cf830e04): 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 |
So it does seem that not reusing the post-order calculation is a 0.2-0.3% instruction count reduction. Which does seem quite significant. I suspect there's likely also some cost in LLVM itself from it trying to produce better machine code in debug builds, which the change to reuse the post-order iterator likely counteracted. |
We can also use somewhat cheaper traversal. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit e3814fd7c64486dfff70935b358e6538eb9698ab with merge 4e646d2a8538dfe57c622be92b6884e6d9eb677c... |
@bors r+ delegate=tmiasko |
📌 Commit 4237a05 has been approved by |
✌️ @tmiasko can now approve this pull request |
@nagisa did you intend to both approve and delegate at the same time? |
Yes, so that in case there's a need to rebase again, @tmiasko can r=me it themselves. I mildly remember them saying they do not have bors-rights yet. |
I see. |
⌛ Testing commit 4237a05 with merge b9634bcbc1755480469605fd59b81c6ec34276cb... |
💥 Test timed out |
dist-powerpc64le-linux hanged while "Updating crates.io index". @bors retry |
⌛ Testing commit 4237a05 with merge 3672dcda95dc42c3b46df09307a0fd2460ed9a9f... |
@bors treeclosed- |
☀️ Test successful - checks-actions |
Traverse blocks in topological sort of dominance partial order, to ensure that
local analyzer correctly identifies locals that are already in static single
assignment form, while avoiding dependency on implicit numeric order of blocks.
When rebuilding the standard library, this change reduces the number of locals
that require an alloca from 62452 to 62348.