Winch: x64 wait, notify and fence
#10092
Conversation
MarinPostma
requested review from
cfallin and
pchickey
and removed request for
a team
|
I can help with this one. |
saulecabrera
requested review from
saulecabrera
and removed request for
cfallin and
pchickey
MarinPostma
force-pushed
the
atomic-wait
branch
from
2b47580 to
42edcd2
Compare
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
winch/codegen/src/codegen/mod.rs
Outdated
| // Put the target memory index in a register, and push it as the first argument. | ||
| let mem = self.context.any_gpr(self.masm)?; | ||
| self.masm.mov( | ||
| writable!(mem), | ||
| RegImm::i32(arg.memory as i32), | ||
| OperandSize::S32, | ||
| )?; | ||
| self.context | ||
| .stack | ||
| .push(TypedReg::new(WasmValType::I32, mem).into()); |
There was a problem hiding this 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.
winch/codegen/src/codegen/mod.rs
Outdated
| // compute the offset if necessary. | ||
| self.masm.extend( | ||
| writable!(addr.reg), | ||
| addr.reg, | ||
| ExtendKind::Unsigned(Extend::I64Extend32), | ||
| )?; |
There was a problem hiding this comment.
I think this extend shouldn't be emitted unconditionally: in the case of memory64, the address is expected to be 64 bits, 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?
There was a problem hiding this comment.
Let me check that again, if I recall correctly I added it because it didn't work like I expected with negative offsets
There was a problem hiding this comment.
nevermind, I must have imagined that...
winch/codegen/src/codegen/mod.rs
Outdated
| // pop the arguments from the stack. | ||
| let count = self.context.pop_to_reg(self.masm, None)?; | ||
| let addr = self.context.pop_to_reg(self.masm, None)?; | ||
|
|
||
| // put the target memrory index in a register, and push it as the first argument. | ||
| let mem = self.context.any_gpr(self.masm)?; | ||
| self.masm.mov( | ||
| writable!(mem), | ||
| RegImm::i32(arg.memory as i32), | ||
| OperandSize::S32, | ||
| )?; | ||
| self.context | ||
| .stack | ||
| .push(TypedReg::new(WasmValType::I32, mem).into()); | ||
|
|
||
| // compute offset if necessary. | ||
| self.masm.extend( | ||
| writable!(addr.reg), | ||
| addr.reg, | ||
| ExtendKind::Unsigned(Extend::I64Extend32), | ||
| )?; |
There was a problem hiding this comment.
Similar comments here regarding the immediate and the stack and also the extend.
|
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. |
This PR implements the following instruction in winch for the x64 backend:
atomic.fencememory.atomic.wait32memory.atomic.wait64memory.atomic.notify