Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winch: x64 atomic stores #9987

Merged
merged 17 commits into from
Jan 15, 2025

Conversation

MarinPostma
Copy link
Contributor

This PR implements x64 store operations:

  • i32.atomic.store8
  • i32.atomic.store16
  • i32.atomic.store
  • i64.atomic.store8
  • i64.atomic.store16
  • i64.atomic.store32
  • i64.atomic.store

#9734

@MarinPostma MarinPostma requested review from a team as code owners January 12, 2025 12:10
@MarinPostma MarinPostma requested review from fitzgen and removed request for a team January 12, 2025 12:10
@github-actions github-actions bot added the winch Winch issues or pull requests label Jan 12, 2025
Copy link

Subscribe to Label Action

cc @saulecabrera

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 .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CodeGen module (e.g., emit_check_alignment) and either calling it before calling emit_compute_heap_addr or enhancing emit_compute_heap_addr to check alignment internally if the load/store is atomic.

For reference, this is how alignment is handled in Cranelift.

// TODO: we don't support 128-bit atomic store yet.
bail!(CodeGenError::unexpected_operand_size());
}
// To stay consistent with cranelift, we emit a normal load followed by a mfence,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// To stay consistent with cranelift, we emit a normal load followed by a mfence,
// To stay consistent with cranelift, we emit a normal store followed by a mfence,

@fitzgen fitzgen removed their request for review January 13, 2025 19:42
@MarinPostma MarinPostma force-pushed the winc-x64-atomic-store branch 2 times, most recently from 0b53b93 to 4dfacab Compare January 13, 2025 20:07
@MarinPostma
Copy link
Contributor Author

@saulecabrera I have added the align check

@MarinPostma MarinPostma force-pushed the winc-x64-atomic-store branch 2 times, most recently from 02fa02a to e462bb3 Compare January 13, 2025 20:31
) -> Result<Option<Reg>> {
if check_align {
self.check_align(memarg, access_size)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally in the codegen module, we try to stick with the emit_* prefix, for consistency, could you update the name of this method?

@@ -648,7 +648,12 @@ where
&mut self,
memarg: &MemArg,
access_size: OperandSize,
check_align: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls emit_check_align and emit_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.

Comment on lines 851 to 853
let addr = *self.context.stack.peek().unwrap();
let tmp = self.context.any_gpr(self.masm)?;
self.context.move_val_to_reg(&addr, tmp, self.masm)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note with using self.context.pop_to_reg is that this method needs to ensure that the value is pushed back to the value stack after all the checks are emitted, to ensure that emit_compute_heap_address is able to pop the address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 offset to the addr, and we cannot do that with the addr as a dst, as we need the address intact for computing the heap address later on. The reason why we can't add with tmp as a dst, is because that will return a InvalidTwoArgumentForm.

In light of this, I think peeking and moving to tmp would save a mov in the case where we need to compute the offset. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a good point yeah. My main concern with peek is 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 doing peek().unwrap()? We have CodeGenError::missing_values_in_stack for this kind of situation.

fn check_align(&mut self, memarg: &MemArg, size: OperandSize) -> Result<()> {
if size.bytes() > 1 {
let addr = *self.context.stack.peek().unwrap();
let tmp = self.context.any_gpr(self.masm)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could use a scratch register here? (scratch!(M))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@saulecabrera saulecabrera removed the request for review from a team January 14, 2025 14:16
@MarinPostma MarinPostma force-pushed the winc-x64-atomic-store branch from 491c1a7 to 26d9841 Compare January 14, 2025 14:58
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

After the trailing comment is deleted, we can land this one.

if size.bytes() > 1 {
let addr = self.context.pop_to_reg(self.masm, None)?;
let tmp = scratch!(M);
// self.context.move_val_to_reg(&addr, tmp, self.masm)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// self.context.move_val_to_reg(&addr, tmp, self.masm)?;

@MarinPostma MarinPostma force-pushed the winc-x64-atomic-store branch from 26d9841 to 25a703c Compare January 14, 2025 15:22
@MarinPostma
Copy link
Contributor Author

@saulecabrera it's not working yet I had to move to a x86 machine to debug, I'll ping you when i fixed it

@MarinPostma
Copy link
Contributor Author

@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.

This was referenced Jan 15, 2025
@MarinPostma MarinPostma force-pushed the winc-x64-atomic-store branch from 73bab48 to cd24682 Compare January 15, 2025 13:04
@MarinPostma
Copy link
Contributor Author

@saulecabrera I made the final changes

@saulecabrera saulecabrera added this pull request to the merge queue Jan 15, 2025
Merged via the queue into bytecodealliance:main with commit 3a4cf0a Jan 15, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants