Stream: git-wasmtime

Topic: wasmtime / PR #3783 [RFC] ISLE: Lowering of multi-output ...

Wasmtime GitHub notifications bot (Feb 09 2022 at 12:01):

uweigand opened PR #3783 from isle-multioutput to main:

This changes the output of the lower constructor from a
ValueRegs to a new InstOutput type, which is a vector
of ValueRegs.

Code in lower_common is updated to use this new type to
handle instructions with multiple outputs. All back-ends
are updated to use the new type.

CC @cfallin @fitzgen - this is an attempt to address https://github.com/bytecodealliance/wasmtime/issues/3747

I like the way lower_common now looks better, and handling of iadd_ifcount seems improved. However, the current implementation requires a large number of changes to all back ends, which is not ideal. Note that if we end up implementing the suggestion https://github.com/bytecodealliance/wasmtime/issues/3753, we can avoid most of those changes simply by installing a default conversion from a ValueRegs to a (singleton) InstOutput.

Not sure if the terminology InstOutput is the best possible -- other suggestions appreciated!

<!--

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 14 2022 at 19:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:11):

fitzgen created PR review comment:

nitpick: Could we call this side_effect? This would match how we have the SideEffect type (not Sideeffect) right now.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:12):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:35):

cfallin created PR review comment:

On general comment -- we should look to make sure in whatever final design we use, we don't incur any allocations in the ordinary one-register case. We use SmallVec in a number of other places in the backend framework, and some of the chosen inline-vector sizes were based on allocation profiling; we found sometimes significant speedups from this at least early in development. I think we can definitely get around this though (see top-level comment).

Wasmtime GitHub notifications bot (Feb 14 2022 at 19:35):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Feb 15 2022 at 13:23):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2022 at 13:23):

uweigand created PR review comment:

Right, this makes sense.

Wasmtime GitHub notifications bot (Feb 24 2022 at 11:47):

uweigand updated PR #3783 from isle-multioutput to main.

Wasmtime GitHub notifications bot (Feb 24 2022 at 19:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2022 at 19:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2022 at 19:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2022 at 19:57):

cfallin created PR review comment:

I don't see any uses of this builder in the current PR; is it required for some later changes, e.g. handling calls with a variable number of returns? (That's fine if so; I just want to make sure it's not a leftover.)

Wasmtime GitHub notifications bot (Feb 24 2022 at 21:58):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2022 at 21:58):

uweigand created PR review comment:

Yes, exactly. This is used in https://github.com/bytecodealliance/wasmtime/pull/3785 here to construct the output for call instructions:
https://github.com/bytecodealliance/wasmtime/pull/3785/files#diff-1eac808500791b38886e17f448d60faa6583f025fef1c810fadc9dfdc04efe37R2275

Wasmtime GitHub notifications bot (Feb 24 2022 at 22:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2022 at 22:02):

cfallin created PR review comment:

Makes sense, thanks!

Wasmtime GitHub notifications bot (Feb 24 2022 at 22:03):

cfallin merged PR #3783.

Last updated: Dec 23 2024 at 12:05 UTC