Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18): Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18):MarinPostma opened PR #10092 from MarinPostma:atomic-wait to bytecodealliance:main:
This PR implements the following instruction in winch for the x64 backend:
atomic.fencememory.atomic.wait32memory.atomic.wait64memory.atomic.notify
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18):MarinPostma requested cfallin for a review on PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18):MarinPostma requested wasmtime-compiler-reviewers for a review on PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18):MarinPostma requested pchickey for a review on PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:18):MarinPostma requested wasmtime-core-reviewers for a review on PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:19):MarinPostma updated PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:24):MarinPostma updated PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:26):saulecabrera commented on PR #10092:
I can help with this one.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:26):saulecabrera requested saulecabrera for a review on PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 18:46):MarinPostma updated PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 19:27):MarinPostma updated PR #10092.
Wasmtime GitHub notifications bot (Jan 23 2025 at 20:44):github-actions[bot] commented on PR #10092:
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 (Jan 26 2025 at 21:53):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Jan 26 2025 at 21:53):saulecabrera created PR review comment:
Given that the target memory is an immediate, and immediate values in the stack don't affect the stack ordering principle (since they are not spilled), instead of moving it to a register, it should be possible to insert the immediate directly, this would save one memory write and load at the builtin call site.
We use such pattern extensively in the table operations.
Wasmtime GitHub notifications bot (Jan 26 2025 at 21:53):saulecabrera created PR review comment:
Similar comments here regarding the immediate and the stack and also the extend.
Wasmtime GitHub notifications bot (Jan 26 2025 at 21:53):saulecabrera created PR review comment:
I think this extend shouldn't be emitted unconditionally: in the case of
memory64, the address is expected to be64bits, so there shouldn't be a need to emit this.Also, 32-bit loads are zero extend by default, so I think we can omit this extend entirely?
Wasmtime GitHub notifications bot (Jan 27 2025 at 08:57):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Jan 27 2025 at 08:57):MarinPostma created PR review comment:
Let me check that again, if I recall correctly I added it because it didn't work like I expected with negative offsets
Wasmtime GitHub notifications bot (Jan 27 2025 at 09:56):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Jan 27 2025 at 09:56):MarinPostma created PR review comment:
nevermind, I must have imagined that...
Wasmtime GitHub notifications bot (Jan 27 2025 at 09:58):MarinPostma updated PR #10092.
Wasmtime GitHub notifications bot (Jan 27 2025 at 10:00):MarinPostma commented on PR #10092:
I made the suggested changes. I think we could avoid popping the stack in the case where we don't have to compute an offset, and just insert the target memory instead, but it complicates the implementation slightly. Happy to do if you think it's better.
Wasmtime GitHub notifications bot (Jan 27 2025 at 10:19):MarinPostma edited a comment on PR #10092:
I made the suggested changes. I think we could avoid popping the stack in the case where we don't have to compute an offset, and just insert the target memory instead, but it complicates the implementation slightly. Happy to do it if you think it's better.
Wasmtime GitHub notifications bot (Jan 27 2025 at 12:52):saulecabrera submitted PR review:
LGTM, thanks.
Wasmtime GitHub notifications bot (Jan 27 2025 at 13:12):saulecabrera merged PR #10092.
Last updated: Feb 28 2025 at 02:27 UTC