jeffcharles opened PR #7531 from jeffcharles:winch-materialize-latent-locals
to bytecodealliance:main
:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Winch has a bug where when setting or teeing a local, and that local is in the value stack but not at the top of the value stack, the local on the stack will pop with an incorrect value because the value of the local is determined lazily (that is, when it's needed). To address that, we can detect if the value stack contains a reference to the local that is not the top-most element, and spill registers and locals into memory if it does, which will materialize the value in the local before the value of the local is changed.
jeffcharles requested wasmtime-compiler-reviewers for a review on PR #7531.
jeffcharles requested abrown for a review on PR #7531.
jeffcharles submitted PR review.
jeffcharles created PR review comment:
I added this because it made some of my debugging a little easier and all the address implementations implement
Debug
.
jeffcharles submitted PR review.
jeffcharles created PR review comment:
I looked at using
peekn
but the iterator it returns is in bottom-most to top-most order and the return type is not a trait that allows me to reverse the order of the entries so I can ignore the top-most element of the stack.
saulecabrera submitted PR review.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Some thoughts on this snippet:
- I was hoping that we would be able to avoid iterating over all the entries in the stack every time we do this check, as it could get quite expensive plus we really don't have to: the stack must hold the invariant that memory entries precede locals and registers, in that sense, if we're able to track down the index of the last memory entry, we can optimize this check so that it only accounts for entries beyond such index.
- Can we make this a method on
Stack
? Something likeStack::has_local(index)
?
jeffcharles submitted PR review.
jeffcharles created PR review comment:
We talked offline.
We can use an
iter().rev().take_while(|v| /* v is not mem or a const */)
approach to avoid iterating over the entire stack. And the stack method I'm adding will be named in such a way that it indicates it does not check the top element. This is because we can safely avoid spilling if it's only the top element that references the local since we're going to pop the top element immediately after performing the check to see if the stack contains the local.
jeffcharles edited PR review comment.
jeffcharles updated PR #7531.
jeffcharles requested saulecabrera for a review on PR #7531.
saulecabrera submitted PR review.
saulecabrera merged PR #7531.
Last updated: Dec 23 2024 at 13:07 UTC