elliottt opened PR #5821 from trevor/fix-postorder-traversal
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
bjorn3 submitted PR review.
bjorn3 created PR review comment:
The stack is not stored in
rpo_number
.
elliottt edited PR #5821 from trevor/fix-postorder-traversal
to main
:
Fix the postorder traversal computed by the
DominatorTree
. It was recording nodes in the wrong order depending on the order child nodes were visited. Consider the following program:function %foo2(i8) -> i8 { block0(v0: i8): brif v0, block1, block2 block1: return v0 block2: jump block1 }
The postorder produced by the previous implementation was:
block2 block1 block0
Which is incorrect, as
block1
is branched to byblock2
. Changing the branch order in the function would also change the postorder result, yielding the expected order withblock1
emitted first.This PR reworks the implementation of
DominatorTree::compute
to produce an order whereblock1
is always returned first, regardless of the branch order in the original program.Co-authored-by: Jamey Sharp <jsharp@fastly.com>
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt updated PR #5821 from trevor/fix-postorder-traversal
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
We use the
SEEN
value to imply something about the state of the stack, we're not trying to suggest that the stack is stored inrpo_number
. Is there a way that this can be rephrased to make this more clear?
elliottt updated PR #5821 from trevor/fix-postorder-traversal
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Missed the line break in this sentence. I thought it was a separate thing. My bad.
elliottt submitted PR review.
elliottt created PR review comment:
Ah, that makes sense! Would indenting it a little more make it read better?
elliottt requested cfallin for a review on PR #5821.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
That could help.
cfallin submitted PR review.
elliottt updated PR #5821 from trevor/fix-postorder-traversal
to main
.
elliottt has enabled auto merge for PR #5821.
elliottt has disabled auto merge for PR #5821.
elliottt updated PR #5821 from trevor/fix-postorder-traversal
to main
.
elliottt has enabled auto merge for PR #5821.
elliottt merged PR #5821.
Last updated: Dec 23 2024 at 12:05 UTC