Stream: git-wasmtime

Topic: wasmtime / PR #9137 Don't force `Multiple` on multi-resu...

Wasmtime GitHub notifications bot (Aug 16 2024 at 20:55):

alexcrichton opened PR #9137 from alexcrichton:new-value-use-state to bytecodealliance:main:

This commit is a result of [discussion on Zulip][Zulip] and is
attempting to fix an issue where some 128-bit instructions aren't fully
benefitting from load sinking on x64. At a high level 128-bit
addition isn't able to sink loads into instructions for halves of the
128-bit operation. At a lower level the reason for this is that
currently all operands of a multiple-result instruction are considered
multiply-used (as each result could be used) which prevents load
sinking.

Operations on 128-bit integers may be coupled with isplit afterwards
which is a multi-result instruction. This then means that the Multiple
state flows backwards to the 128-bit operation and all its operands,
including whatever is necessary to produce the individual components of
each 128-bit integer.

The fix in this commit is to introduce the concept of a "root"
instruction from the perspective of the calculation of ValueUseState.
In other words ValueUseState is no longer an accurate picture of the
function as a whole, but only the parts of the function rooted at a
"root" instruction. This is currently defined as multi-result
instructions meaning that isplit for example is a root instruction.
This is coupled with documentation/changes to
get_value_as_source_or_const to never allow looking through root
instructions (or considering them a UniqueUse).

This commit additionally updates some documentation in a few places and
refactors some usage of get_value_as_source_or_const to use other
helpers instead to reduce callers of get_value_as_source_or_const and
who to audit when modifying this function.

[Zulip]: https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/ValueUseState.3A.3AMultiple.20and.20multi-result.20instructions/near/462833578

Wasmtime GitHub notifications bot (Aug 16 2024 at 20:55):

alexcrichton requested fitzgen for a review on PR #9137.

Wasmtime GitHub notifications bot (Aug 16 2024 at 20:55):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9137.

Wasmtime GitHub notifications bot (Aug 16 2024 at 20:56):

cfallin requested cfallin for a review on PR #9137.

Wasmtime GitHub notifications bot (Aug 16 2024 at 21:10):

cfallin created PR review comment:

I would maybe rephrase as:

"Notably, we define some instructions to be "root" instructions, which means that we assume they will always be codegen'd at the root of a matching tree, and not matched. (This comes with the caveat that we actually enforce this property by making them "opaque" to subtree matching in get_value_as_source_or_const.) Because they will always be codegen'd once, they in some sense "reset" multiplicity: these root instructions can be used many times, but because their result(s) are only computed once, they only use theirinputs once.

We currently define all multi-result instructions to be "root" instructions, because it is too complex to reason about matching through them, and they cause too-coarse-grained approximation of multiplicity otherwise: the analysis would have to assume (as it used to!) that they are always multiply-used, simply because they have multiple outputs even if those outputs are used only once.

In the future we could define other instructions to be "root" instructions as well, if we make the corresponding change to get_value_as_source_or_const as well."

Wasmtime GitHub notifications bot (Aug 16 2024 at 21:10):

cfallin submitted PR review:

Looks great; thanks for the careful thinking through this and all the additional documentation!

Some edits to my own taste on one of the comments, and a note on possibly generalizing a case, but nothing major below; LGTM.

Wasmtime GitHub notifications bot (Aug 16 2024 at 21:10):

cfallin created PR review comment:

This is asserting that (in effect) we don't have any multiple-result instructions that are side-effecting, I think. That may be true today but I wouldn't want to implicitly bake it in -- could we add !is_value_use_root(...) as an additional condition to this if-statement instead?

Wasmtime GitHub notifications bot (Aug 16 2024 at 23:44):

github-actions[bot] commented on PR #9137:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:01):

alexcrichton updated PR #9137.

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:02):

alexcrichton created PR review comment:

I mostly wanted to make sure that the preexisting self.num_outputs(src_inst) == 1 on this case didn't become accidentally load-bearing so I put this here to ensure that if is_value_use_root changed that it would alert that this needs to be changed as well. That being said I like your suggestion as well, so I've done a bit of refactoring to lift is_value_use_root(...) checking to the top of the conditional to handle both pure/impure cases at the same time.

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2024 at 15:52):

cfallin created PR review comment:

Nice!

Wasmtime GitHub notifications bot (Aug 19 2024 at 16:09):

cfallin merged PR #9137.

Last updated: Nov 22 2024 at 17:03 UTC