Replace a recursive algorithm with an iterative one and a stack. #74983
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue Seems not unlikely to have some perf effects. |
|
Awaiting bors try build completion |
|
⌛ Trying commit ac5b2b0a2a6d526901b412f07651885be3c7f1f9 with merge 7ecf4177219d2b3082e0d5ee04edff8af3e1e80b... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 7ecf4177219d2b3082e0d5ee04edff8af3e1e80b with parent 62f9aa9, future comparison URL. |
There was a problem hiding this comment.
Thanks for doing this @oli-obk. I remember looking at this code early on and wondering if it was going to ruin someone's day 😄. This is a pretty straightforward translation of the recursive version, so I feel good about it even though I left some questions.
| self.pred_count[*target] += 1; | ||
| self.pred_count[current] -= 1; | ||
| } | ||
| *changed |= current != *target; |
There was a problem hiding this comment.
Preexisting I know, but why do we only mark the MIR as changed if current != target? If current == target then we are inserting a self-loop, but it's possible for this to represent a change in the MIR if we had a cycle involving several Gotos.
There was a problem hiding this comment.
I have learned the hard way that tinkering with this code is not a good idea XD But I've also learned that you either break it completely or not at all, there's no middle ground, so I guess i can do more experimentation
There was a problem hiding this comment.
oh well, it worked :D
There was a problem hiding this comment.
If you don't wanna lump it into this PR, that's fine too. It just didn't make sense to me.
There was a problem hiding this comment.
it's already in this PR ;)
| self.pred_count[*start] -= 1; | ||
| // Collapse a goto chain starting from `start` | ||
| fn collapse_goto_chain(&mut self, start: &mut BasicBlock, changed: &mut bool) { | ||
| let mut terminators: SmallVec<[_; 1]> = Default::default(); |
There was a problem hiding this comment.
Why the SmallVec here? In the common case, start will not refer to a Goto with no statements, so terminators will be empty.
There was a problem hiding this comment.
anecdotal evidence showed me many single-step goto chains (I had logging on during libcore builds). But we can also just go ahead and benchmark Vec vs SmallVec<[_; 1]> and maybe some other default sizes.
There was a problem hiding this comment.
I'm satisfied with that. It shouldn't matter much either way, maybe leave a comment?
|
Finished benchmarking try commit (7ecf4177219d2b3082e0d5ee04edff8af3e1e80b): 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 |
oli-obk
force-pushed
the
mir_opt_goto_chain
branch
from
49d0c9a to
aba0251
Compare
|
@bors r=ecstatic-morse |
|
📌 Commit aba0251 has been approved by |
bors
added
S-waiting-on-bors
…arth Rollup of 6 pull requests Successful merges: - rust-lang#74977 (Clean up E0741 error explanation) - rust-lang#74981 (Some fixes for `plugin.md` in unstable-book) - rust-lang#74983 (Replace a recursive algorithm with an iterative one and a stack.) - rust-lang#74995 (Update the WASI libc build to LLVM 10.) - rust-lang#74996 (submodules: update cargo from 974eb438d to 2d5c2381e) - rust-lang#75007 (Clean up E0743 explanation) Failed merges: r? @ghost
fixes #74931