Use a more efficient iteration order for forward dataflow #62062
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@nagisa. Do you think the assertion about unreachable basic blocks is correct? I added it because there could be two successive basic blocks which are unreachable from the start block, in which case the entry set for the second one would be wrong. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ecstatic-morse
force-pushed
the
dataflow-order
branch
from
1d26fb0
to
b03ebd5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors try |
|
⌛ Trying commit b03ebd53e5b427b95fa36b0ec295b72125d7e371 with merge 0905d6a630cb4afc3f894f9e91c1a7a20c32416b... |
|
☀️ Try build successful - checks-travis |
|
@rust-timer build 0905d6a630cb4afc3f894f9e91c1a7a20c32416b |
|
Success: Queued 0905d6a630cb4afc3f894f9e91c1a7a20c32416b with parent d6884ae, comparison URL. |
|
Finished benchmarking try commit 0905d6a630cb4afc3f894f9e91c1a7a20c32416b, comparison URL. |
|
This seems ... fine? It doesn't seems to hurt anything; it also doesn't improve things terribly much, probably because the block numbering was close to RPO already, as hypothesized. ts probably not a good idea to implicitly rely on the block numbering always remaining close to RPO, right? And therefore we should land this? What do you think, @ecstatic-morse ? (@nagisa and @arielb1 may also have thoughts on this, since I know they've each also spent time thinking about or working on dataflow analyses.) |
|
I think this should probably land (totally not biased or anything :). It's a sensible default, the overhead of a single extra RPO traversal per dataflow analysis is pretty small, and it removes the implicit dependency on a certain basic block ordering, which may change as more MIR transformations are added. With the addition of more expensive dataflow passes (i.e. reaching definitions), the naive ordering could eventually have an observable performance impact. The alternative is to wait until a MIR transformation is actually added which renumbers basic blocks, but this PR will probably be long forgotten by then 😄. |
|
Also, this does reduce the number of dataflow iterations, just not enough to matter. I observed a reduction of 9% across all dataflow analyses which took more than 5 iterations when running the tests in |
Currently, dataflow begins by visiting each block in order of ID (`BasicBlock(0)`, `BasicBlock(1)`, etc.). This PR changes that initial iteration to reverse post-order. This ensures that the effects of all predecessors will be applied before a basic block is visited if the CFG has no back-edges, and should result in less total iterations even when back-edges exist. This should not change the results of dataflow analysis. The current ordering for basic blocks is pretty close to RPO already--`BasicBlock(0)` is already the start block, so the gains from this are pretty small, especially since we need to do an extra traversal up front. Note that some basic blocks are unreachable from the `START_BLOCK` during dataflow. We add these blocks to the work queue as well to preserve the original behavior.
ecstatic-morse
force-pushed
the
dataflow-order
branch
from
b03ebd5
to
07c5e2b
Compare
This applies the same basic principle as rust-lang#62062 to the reverse dataflow analysis used to compute liveness information. It is functionally equivalent, except that post-order is used instead of reverse post-order. Some `mir::Body`s contain basic blocks which are not reachable from the `START_BLOCK`. We need to add them to the work queue as well to preserve the original semantics.
|
I think it's OK to land this to avoid the dependence on basic block ordering. |
|
@bors r+ |
|
📌 Commit 07c5e2b has been approved by |
bors
added
S-waiting-on-bors
…gisa Use a more efficient iteration order for forward dataflow Currently, dataflow begins by visiting each block in order of ID (`BasicBlock(0)`, `BasicBlock(1)`, etc.). This PR changes that initial iteration to reverse post-order (see [this blog post](https://eli.thegreenplace.net/2015/directed-graph-traversal-orderings-and-applications-to-data-flow-analysis/#data-flow-analysis) for more info). This ensures that the effects of all predecessors will be applied before a basic block is visited if the CFG has no back-edges, and should result in less total iterations even when back-edges exist. This should not change the results of dataflow analysis. The current ordering for basic blocks may be pretty close to RPO already--`BasicBlock(0)` is already the start block--in which case the cost of doing the traversal up front will outweigh the efficiency gains. A perf run is needed to check this. r? @pnkfelix (I think).
…der, r=nagisa Use a more efficient iteration order for backward dataflow This applies the same basic principle as rust-lang#62062 to the reverse dataflow analysis used to compute liveness information. It is functionally equivalent, except that post-order is used instead of reverse post-order. In the long-term, `BitDenotation` should probably be extended to support both forward and backward dataflow, but there's some more work needed to get to that point.
Rollup of 8 pull requests Successful merges: - #62062 (Use a more efficient iteration order for forward dataflow) - #62063 (Use a more efficient iteration order for backward dataflow) - #62224 (rustdoc: remove unused derives and variants) - #62228 (Extend the #[must_use] lint to boxed types) - #62235 (Extend the `#[must_use]` lint to arrays) - #62239 (Fix a typo) - #62241 (Always parse 'async unsafe fn' + properly ban in 2015) - #62248 (before_exec actually will only get deprecated with 1.37) Failed merges: r? @ghost
Currently, dataflow begins by visiting each block in order of ID (
BasicBlock(0),BasicBlock(1), etc.). This PR changes that initial iteration to reverse post-order (see this blog post for more info). This ensures that the effects of all predecessors will be applied before a basic block is visited if the CFG has no back-edges, and should result in less total iterations even when back-edges exist. This should not change the results of dataflow analysis.The current ordering for basic blocks may be pretty close to RPO already--
BasicBlock(0)is already the start block--in which case the cost of doing the traversal up front will outweigh the efficiency gains.A perf run is needed to check this.
r? @pnkfelix (I think).