Stream: git-wasmtime

Topic: wasmtime / PR #7517 Get addr of local after popping from reg

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:09):

jeffcharles opened PR #7517 from jeffcharles:winch-get-addr-after-popping-to-reg 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
-->
I observed in Winch that the offset from the stack pointer for locals written to with local.set and local.tee are slightly wrong when they are writing to a local that had been spilled by a prior operation. The cause appears to be that we get the address of the local (that is, the offset from the stack pointer) before invoking self.context.pop_to_reg and then use that address to perform the store operation. self.context.pop_to_reg may change the value of the stack pointer if the value on top of the stack is in memory. So the previously calculated stack pointer offset of the local may not be correct when storing a value into that address. This change has us calculate the address of the local (that is, the offset from the stack pointer) after invoking self.context.pop_to_reg to avoid that problem.

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:09):

jeffcharles requested fitzgen for a review on PR #7517.

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:09):

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

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:16):

saulecabrera submitted PR review:

Thanks for finding this, I left one minor nit on wording, but else this looks good to me.

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:16):

saulecabrera submitted PR review:

Thanks for finding this, I left one minor nit on wording, but else this looks good to me.

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:16):

saulecabrera created PR review comment:

One minor nit on this comment: it won't change the local's offset, it will change the calculation of that offset from the current position of the stack pointer. I'd suggest rewording this a bit to something like: _Need to get the local address before popping the value given that pop_to_reg will pop the machine stack, causing a wrong address to be calculated_

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:24):

jeffcharles updated PR #7517.

Wasmtime GitHub notifications bot (Nov 09 2023 at 20:28):

saulecabrera has enabled auto merge for PR #7517.

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

saulecabrera merged PR #7517.

Last updated: Dec 23 2024 at 12:05 UTC