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 theMultiple
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 ofValueUseState
.
In other wordsValueUseState
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 thatisplit
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 aUniqueUse
).This commit additionally updates some documentation in a few places and
refactors some usage ofget_value_as_source_or_const
to use other
helpers instead to reduce callers ofget_value_as_source_or_const
and
who to audit when modifying this function.
alexcrichton requested fitzgen for a review on PR #9137.
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9137.
cfallin requested cfallin for a review on PR #9137.
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."
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.
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?
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>
alexcrichton updated PR #9137.
alexcrichton submitted PR review.
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 ifis_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 liftis_value_use_root(...)
checking to the top of the conditional to handle both pure/impure cases at the same time.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Nice!
cfallin merged PR #9137.
Last updated: Dec 23 2024 at 12:05 UTC