Stream: git-wasmtime

Topic: wasmtime / PR #5750 Reuse `inst_predicates::visit_block_s...


view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:01):

elliottt opened PR #5750 from trevor/visit-blocks to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:02):

elliottt edited PR #5750 from trevor/visit-blocks to main:

Following up from #5730, replace some explicit matching over branch instructions with a use of inst_predicates::visit_block_succs.

cc @bjorn3

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:02):

elliottt requested jameysharp for a review on PR #5750.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:05):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:05):

elliottt created PR review comment:

Using visit_block_succs changes the traversal order here, traversing the default block first.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:07):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:08):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 22:08):

bjorn3 created PR review comment:

I think that is fine, but I am not familiar with this code.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:08):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:08):

jameysharp created PR review comment:

The doc comment for this function says the requirement is that "if a block is split in two, we get the same post-order except for the insertion of the new block header at the split point" (which it calls a "split-invariant post-order"). As far as I can tell, the order of visiting successors of a single instruction doesn't affect that property, as long as the order is stable if you compute the dominator tree multiple times.

Looking further at the history, I think a lot of this is remnants from the old "extended basic block" model. There's a giant comment in compute_postorder, which is immediately above this function, where as far as I can tell almost none of that comment applies to Cranelift today. Most of that comment was written when this was still called Cretonne.

I wouldn't be shocked if we don't care about "split-invariant post-orders" at all any more. Even if we do, I don't think this change breaks that property. But just in case I'm wrong about both of those, let's see if @cfallin wants to weigh in.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:35):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:35):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:35):

cfallin created PR review comment:

Yeah, I don't think traversal order should matter here; and if it does, to the extent that it causes a bug, we should fix that.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 23:42):

elliottt merged PR #5750.


Last updated: Dec 23 2024 at 12:05 UTC