Stream: git-wasmtime

Topic: wasmtime / PR #5740 Make `DataFlowGraph::blocks` public

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

elliottt opened PR #5740 from trevor/pub-blocks to main:

Similar to when we exposed the DataFlowGraph::insts field through a restrictive newtype, expose DataFlowGraph::blocks through an interfac that allows a restrictive set of operations. This field being public now allows us to avoid a rematch in ssa construction, and simplifies the implementation of adding a block argument to a block referenced by a br_table instruction.
<!--

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 07 2023 at 22:07):

elliottt requested jameysharp for a review on PR #5740.

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

elliottt updated PR #5740 from trevor/pub-blocks to main.

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

elliottt requested cfallin for a review on PR #5740.

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

jameysharp submitted PR review.

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

jameysharp submitted PR review.

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

jameysharp created PR review comment:

Others may disagree with me here but I don't think there's value in the num_block_params interface. Getting the slice of block params doesn't cost any more than getting its length (I think it adds 4 to a pointer it already computed to get the length, and returns the adjusted pointer along with the length) so I don't think the extra API clutter is worth its weight.

We can leave DataFlowGraph::num_block_params so we aren't forcing people to change their calls for no reason, but I'd rather not add this function in this new spot.

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

jameysharp created PR review comment:

Bikeshed: Since make_block is already accessed through a field named "blocks", how about a name that doesn't have "block" in it? I was tempted to suggest blocks.new() but that's not exactly idiomatic, so maybe just blocks.add()?

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

elliottt submitted PR review.

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

elliottt created PR review comment:

I like that!

Wasmtime GitHub notifications bot (Feb 07 2023 at 23:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 23:53):

elliottt created PR review comment:

That sounds good to me!

Wasmtime GitHub notifications bot (Feb 07 2023 at 23:54):

elliottt updated PR #5740 from trevor/pub-blocks to main.

Wasmtime GitHub notifications bot (Feb 07 2023 at 23:54):

elliottt requested jameysharp for a review on PR #5740.

Wasmtime GitHub notifications bot (Feb 08 2023 at 00:28):

elliottt updated PR #5740 from trevor/pub-blocks to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 00:29):

elliottt updated PR #5740 from trevor/pub-blocks to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 00:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 00:59):

elliottt edited PR #5740 from trevor/pub-blocks to main:

Similar to when we exposed the DataFlowGraph::insts field through a restrictive newtype, expose DataFlowGraph::blocks through an interface that allows a restrictive set of operations. This field being public now allows us to avoid a rematch in ssa construction, and simplifies the implementation of adding a block argument to a block referenced by a br_table instruction.
<!--

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 01:11):

elliottt merged PR #5740.

Last updated: Dec 23 2024 at 12:05 UTC