Wasmtime GitHub notifications bot (Jan 12 2025 at 12:10): Wasmtime GitHub notifications bot (Jan 12 2025 at 12:10):MarinPostma opened PR #9987 from MarinPostma:winc-x64-atomic-store to bytecodealliance:main:
This PR implements x64 store operations:
i32.atomic.store8i32.atomic.store16i32.atomic.storei64.atomic.store8i64.atomic.store16i64.atomic.store32i64.atomic.store
Wasmtime GitHub notifications bot (Jan 12 2025 at 12:10):MarinPostma requested wasmtime-compiler-reviewers for a review on PR #9987.
Wasmtime GitHub notifications bot (Jan 12 2025 at 12:10):MarinPostma requested fitzgen for a review on PR #9987.
Wasmtime GitHub notifications bot (Jan 12 2025 at 12:10):MarinPostma requested wasmtime-core-reviewers for a review on PR #9987.
Wasmtime GitHub notifications bot (Jan 12 2025 at 13:44):github-actions[bot] commented on PR #9987:
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 12 2025 at 16:00):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 14:15):saulecabrera submitted PR review:
One thing that we might want to do as part of this change before proceeding further with the development of the rest of the instructions in the proposal, is to ensure that addresses are aligned for atomic loads/stores. See the alignment of the proposal.
To check for alignment, I'd recommend creating a new method in the
CodeGenmodule (e.g.,emit_check_alignment) and either calling it before callingemit_compute_heap_addror enhancingemit_compute_heap_addrto check alignment internally if the load/store is atomic.For reference, this is how alignment is handled in Cranelift.
Wasmtime GitHub notifications bot (Jan 13 2025 at 14:15):saulecabrera created PR review comment:
// To stay consistent with cranelift, we emit a normal store followed by a mfence,
Wasmtime GitHub notifications bot (Jan 13 2025 at 15:22):saulecabrera submitted PR review:
One thing that we might want to do as part of this change before proceeding further with the development of the rest of the instructions in the proposal, is to ensure that addresses are aligned for atomic loads/stores. See the alignment section of the proposal.
To check for alignment, I'd recommend creating a new method in the
CodeGenmodule (e.g.,emit_check_alignment) and either calling it before callingemit_compute_heap_addror enhancingemit_compute_heap_addrto check alignment internally if the load/store is atomic.For reference, this is how alignment is handled in Cranelift.
Wasmtime GitHub notifications bot (Jan 13 2025 at 18:09):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 19:53):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:07):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:09):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:20):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:21):MarinPostma commented on PR #9987:
@saulecabrera I have added the align check
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:26):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:28):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:31):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera created PR review comment:
Generally in the codegen module, we try to stick with the
emit_*prefix, for consistency, could you update the name of this method?
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera created PR review comment:
What do you think of either:
- Passing an enum here describing the type of heap address computation: (
HeapAddress::AlignChecked,HeapAddress::AlignUnchecked)- Creating a wrapper method
emit_compute_heap_address_align_checked, which internally callsemit_check_alignandemit_compute_heap_address?Heap address calculation is a very critical piece of the compiler, so I'd recommend against passing boolean params, to make it less error prone at call sites.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera created PR review comment:
I believe you could use a scratch register here? (
scratch!(M))
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera created PR review comment:
One thing to note with using
self.context.pop_to_regis that this method needs to ensure that the value is pushed back to the value stack after all the checks are emitted, to ensure thatemit_compute_heap_addressis able to pop the address.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:16):saulecabrera created PR review comment:
I'd recommend using
self.context.pop_to_reg, which already handles all the cases for moving a value to a register; e.g., in this case if a the value is already a register, this code will emit a move, which could be avoided, and more importantly it'll reduce register pressure.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:54):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 14 2025 at 14:58):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 14 2025 at 15:21):saulecabrera submitted PR review:
Looks good, thanks!
After the trailing comment is deleted, we can land this one.
Wasmtime GitHub notifications bot (Jan 14 2025 at 15:21):saulecabrera created PR review comment:
Wasmtime GitHub notifications bot (Jan 14 2025 at 15:22):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 14 2025 at 16:05):MarinPostma commented on PR #9987:
@saulecabrera it's not working yet I had to move to a x86 machine to debug, I'll ping you when i fixed it
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:13):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:13):MarinPostma created PR review comment:
I'm a bit worried about the risk of clobbering the a scratch register here. The register is used across multiple masm operation, and we do use scratch registers in masm often.
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:23):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:23):MarinPostma created PR review comment:
I've been playing around with that, but it turns out that we need to move the address to a register anyway, because we potentially need to add the
offsetto theaddr, and we cannot do that with theaddras adst, as we need the address intact for computing the heap address later on. The reason why we can't add withtmpas a dst, is because that will return aInvalidTwoArgumentForm.In light of this, I think peeking and moving to
tmpwould save amovin the case where we need to compute the offset. WDYT?
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:31):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 14 2025 at 17:44):MarinPostma commented on PR #9987:
@saulecabrera, I have made the changes you requested, I also responded to your comments, let me know if you want me to make the changes I suggested in the comments instead.
Wasmtime GitHub notifications bot (Jan 14 2025 at 20:55):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Jan 14 2025 at 20:55):saulecabrera created PR review comment:
Oh that's a good point yeah. My main concern with
peekis that nothing is preventing any other method to accidentally pop the wrong value from the stack. However, since this pattern is local to this method, I don't think there's a huge risk, so yeah, let's try the peek approach. One comment to your original implementation, could you return an error instead of doingpeek().unwrap()? We haveCodeGenError::missing_values_in_stackfor this kind of situation.
Wasmtime GitHub notifications bot (Jan 15 2025 at 12:44):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 15 2025 at 13:04):MarinPostma updated PR #9987.
Wasmtime GitHub notifications bot (Jan 15 2025 at 13:05):MarinPostma commented on PR #9987:
@saulecabrera I made the final changes
Wasmtime GitHub notifications bot (Jan 15 2025 at 13:41):saulecabrera merged PR #9987.
Last updated: Jan 24 2025 at 00:11 UTC