Handle irreducible control flow in branch simplification#13385
Open
fitzgen wants to merge 1 commit into
Open
Conversation
We previously removed a block from the CFG if it was not marked as reachable by the time the egraph pass visited it. The pass's traversal is both (1) a depth-first pre-order dominator traversal, and (2) a reverse post-order CFG traversal. This traversal visits all of a block's non-back edge predecessors before visiting the block itself. For reducible control flow, this is all that is necessary because we've already visited every back edge's target block already. However, for irreducible control flow, blocks can be reachable *only* through back edges, and so the traversal's property alone was not sufficient. (The `EgraphBlockIter`'s proof is still correct, at least to the best of my knowledge since we haven't mechanically proven it, but the implicit assumption that its proven property is sufficient for our reachability-based block removal is incorrect in the face of irreducible control flow.) This commit's fix is to only remove blocks when they haven't been marked reachable *and* all of its predecessors have been visited (this latter bit being the thing that irreducible control flow broke). To implement this, we pass in the already-computed `ControlFlowGraph` from the `Context` into the `EgraphPass` so that we can easily iterate of a block's predecessors. Fixes bytecodealliance#13365
Member
|
I guess I come back to the point I made here: we now have a pretty complex approach (with a very intricate proof, to your credit!) that only mostly satisfies the requirements, but (with this patch) will sometimes not fully DCE dead branches. On the other hand, if we had a separate DFS-over-blocks to find reachable blocks, we would (with a pretty simple implementation we could be pretty certain about) have correct reachable-code computation in all cases, without having to conservatively over-approximate and leave some dead code in place. Perhaps we should consider that approach again? Can we at least measure its compile-time impact? |
Member
Author
|
Yeah, I thought about that too. I'll investigate a little. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We previously removed a block from the CFG if it was not marked as reachable by the time the egraph pass visited it. The pass's traversal is both (1) a depth-first pre-order dominator traversal, and (2) a reverse post-order CFG traversal. This traversal visits all of a block's non-back edge predecessors before visiting the block itself. For reducible control flow, this is all that is necessary because we've already visited every back edge's target block already.
However, for irreducible control flow, blocks can be reachable only through back edges, and so the traversal's property alone was not sufficient. (The
EgraphBlockIter's proof is still correct, at least to the best of my knowledge since we haven't mechanically proven it, but the implicit assumption that its proven property is sufficient for our reachability-based block removal is incorrect in the face of irreducible control flow.)This commit's fix is to only remove blocks when they haven't been marked reachable and all of its predecessors have been visited (this latter bit being the thing that irreducible control flow broke). To implement this, we pass in the already-computed
ControlFlowGraphfrom theContextinto theEgraphPassso that we can easily iterate of a block's predecessors.Fixes #13365