Stream: git-wasmtime

Topic: wasmtime / PR #7531 winch: Materialize latent locals when...

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:22):

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.html

Please 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.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:22):

jeffcharles requested wasmtime-compiler-reviewers for a review on PR #7531.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:22):

jeffcharles requested abrown for a review on PR #7531.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:23):

jeffcharles submitted PR review.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:23):

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.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:25):

jeffcharles submitted PR review.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:25):

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.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:35):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:35):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 13 2023 at 19:35):

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 like Stack::has_local(index)?

Wasmtime GitHub notifications bot (Nov 13 2023 at 21:09):

jeffcharles submitted PR review.

Wasmtime GitHub notifications bot (Nov 13 2023 at 21:09):

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.

Wasmtime GitHub notifications bot (Nov 13 2023 at 21:11):

jeffcharles edited PR review comment.

Wasmtime GitHub notifications bot (Nov 13 2023 at 21:32):

jeffcharles updated PR #7531.

Wasmtime GitHub notifications bot (Nov 13 2023 at 21:33):

jeffcharles requested saulecabrera for a review on PR #7531.

Wasmtime GitHub notifications bot (Nov 14 2023 at 11:46):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 14 2023 at 12:30):

saulecabrera merged PR #7531.

Last updated: Nov 22 2024 at 17:03 UTC