Stream: git-wasmtime

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

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.

• [ ] 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.
-->

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.

• [ ] 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.
-->

Wasmtime GitHub notifications bot (Feb 08 2023 at 22:02):

elliottt requested jameysharp for a review on PR #5750.

Wasmtime GitHub notifications bot (Feb 08 2023 at 22:05):

elliottt submitted PR review.

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.

Wasmtime GitHub notifications bot (Feb 08 2023 at 22:07):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 22:08):

bjorn3 submitted PR review.

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.

Wasmtime GitHub notifications bot (Feb 08 2023 at 23:08):

jameysharp submitted PR review.

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.

Wasmtime GitHub notifications bot (Feb 08 2023 at 23:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 23:35):

cfallin submitted PR review.

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.

Wasmtime GitHub notifications bot (Feb 08 2023 at 23:42):

elliottt merged PR #5750.

Last updated: Oct 23 2024 at 20:03 UTC