Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24): Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera requested cfallin for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera opened PR #6912 from saulecabrera:winch-parametric to bytecodealliance:main:
This commit introduces support for the
dropandselectinstructions.Additionally, it refactors the
CodeGenContext::drop_lastimplementation, enhancing flexibility for callers to determine the handling of elements to be dropped. This refactoring simplifies scenarios where aMemoryentry is at the top of the stack.<!--
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
-->
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera requested fitzgen for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera requested wasmtime-core-reviewers for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:24):saulecabrera requested wasmtime-core-reviewers for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:26):saulecabrera created PR review comment:
I'm planning on doing this refactor, but I didn't want to introduce it as part of this change.
Wasmtime GitHub notifications bot (Aug 25 2023 at 14:26):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Aug 25 2023 at 15:53):cfallin submitted PR review:
Generally looks good, thanks! Just a few questions below.
Wasmtime GitHub notifications bot (Aug 25 2023 at 15:53):cfallin submitted PR review:
Generally looks good, thanks! Just a few questions below.
Wasmtime GitHub notifications bot (Aug 25 2023 at 15:53):cfallin created PR review comment:
I'm somewhat surprised that select is being implemented with actual local control flow here; is there any reason we can't use a conditional move? That's the usual lowering in e.g. Cranelift (and avoiding actual branches is sometimes important at the guest-code level for Spectre mitigations).
Wasmtime GitHub notifications bot (Aug 25 2023 at 15:53):cfallin created PR review comment:
It's not entirely clear what's going on in the diff here (i.e. why things changed): is it that the registers weren't actually freed before?
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:27):saulecabrera updated PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:28):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:28):saulecabrera created PR review comment:
is there any reason we can't use a conditional move?
Good catch; no particular reason why we can't use a conditional move here, it's just that I didn't consider it initially, but there are very good arguments to do so. I've refactored this code to use conditional moves instead in https://github.com/bytecodealliance/wasmtime/pull/6912/commits/c56125cd833ff8107b285f4a2872d0930c014eef
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:33):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:33):saulecabrera created PR review comment:
The registers were actually freed, implicitly as part of the previous implementation of the
drop_lastfunction, with the new implementation, the processing of certain values like registers and memory are explicit and up to the caller. I noticed that this change to thedrop_lastfunction has the potential to reduce a bit the boilerplate in calls, hence theNOTE / TODOcomment. Hope this clarifies things up.
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:33):saulecabrera requested cfallin for a review on PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:36):cfallin submitted PR review:
LGTM!
Wasmtime GitHub notifications bot (Aug 25 2023 at 17:36):cfallin has enabled auto merge for PR #6912.
Wasmtime GitHub notifications bot (Aug 25 2023 at 18:36):cfallin merged PR #6912.
Last updated: Dec 23 2024 at 13:07 UTC