Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56): Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56):jeffcharles requested cfallin for a review on PR #10226.
Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56):jeffcharles requested wasmtime-compiler-reviewers for a review on PR #10226.
Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56):jeffcharles opened PR #10226 from jeffcharles:winch-simd-trunc-sat 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
-->
Part of #8093. Adds implementations for the following instructions:
i32x4.trunc_sat_f32x4_si32x4.trunc_sat_f32x4_ui32x4.trunc_sat_f64x2_s_zeroi32x4.trunc_sat_f64x2_u_zero
Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56):jeffcharles requested pchickey for a review on PR #10226.
Wasmtime GitHub notifications bot (Feb 12 2025 at 20:56):jeffcharles requested wasmtime-core-reviewers for a review on PR #10226.
Wasmtime GitHub notifications bot (Feb 12 2025 at 22:45):github-actions[bot] commented on PR #10226:
Subscribe to Label Action
cc @saulecabrera
<details>
This issue or pull request has been labeled: "winch"Thus the following users have been cc'd because of the following labels:
- saulecabrera: winch
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Feb 13 2025 at 12:50):saulecabrera created PR review comment:
Each arm here is considerably long, could we extract each into it's own helper? I had a bit of a hard time following along, having each arm in its own function will make it easier (at least personally) to reason about each invariant.
Wasmtime GitHub notifications bot (Feb 13 2025 at 12:50):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 12:50):saulecabrera created PR review comment:
Usually
CodeGenContextis passed when there's a special ISA needs for lowering, but I don't think that the case here? If I'm reading the code correctly, it seems that the only reason why we need the context is to pop a temporary register when kind ==F32x4U?If that's the case, could we instead avoid passing the context and pass an
Option<Reg>at each call site where appropriate?
Wasmtime GitHub notifications bot (Feb 13 2025 at 12:50):saulecabrera created PR review comment:
I'm confused about the lifetime of this register, I was expecting a
context.free_reggiven that it seems to be temporary?
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:12):jeffcharles submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:12):jeffcharles created PR review comment:
Would we still want to allocate the extra register on AArch64 for
F32x4Uwhen it isn't necessary on that ISA?Otherwise I'm not sure how to handle only allocating the register on x64 if we're doing the allocation outside the x64 macroassembler.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:13):jeffcharles submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:13):jeffcharles created PR review comment:
Good catch!
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:33):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:33):saulecabrera created PR review comment:
Would we still want to allocate the extra register on AArch64 for F32x4U when it isn't necessary on that ISA?
I was under the impression that we'd still need an extra reg for aarch64, is that not the case? More generally, maybe it's fine to leave it as is and we can refactor this once we add SIMD support for other backends.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:47):jeffcharles updated PR #10226.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:50):jeffcharles submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 20:50):jeffcharles created PR review comment:
AFAICT, we could implement this instruction with just a single emission of FCVTZU on AArch64 and we would only need the one register.
Wasmtime GitHub notifications bot (Feb 13 2025 at 21:52):jeffcharles updated PR #10226.
Wasmtime GitHub notifications bot (Feb 13 2025 at 21:53):jeffcharles requested saulecabrera for a review on PR #10226.
Wasmtime GitHub notifications bot (Feb 13 2025 at 23:25):saulecabrera submitted PR review:
LGTM, thanks!
Wasmtime GitHub notifications bot (Feb 13 2025 at 23:45):saulecabrera merged PR #10226.
Last updated: Feb 28 2025 at 01:30 UTC