Winch: x64 atomic CAS #10039
Winch: x64 atomic CAS #10039
Conversation
MarinPostma
force-pushed
the
winch-x64-rmw-cmpxchg
branch
from
ec5d429 to
d86981b
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 |
|
I'll take a look at this one. |
saulecabrera
requested review from
saulecabrera
and removed request for
a team and
fitzgen
MarinPostma
force-pushed
the
winch-x64-rmw-cmpxchg
branch
from
d86981b to
02ff251
Compare
There was a problem hiding this comment.
I believe we can now enable the remaining spec tests?
wasmtime/crates/wast-util/src/lib.rs
Line 373 in 02ff251
winch/codegen/src/codegen/mod.rs
Outdated
| // pop the args | ||
| let replacement = self | ||
| .context | ||
| .stack | ||
| .pop() | ||
| .ok_or_else(CodeGenError::missing_values_in_stack)?; | ||
| let expected = self | ||
| .context | ||
| .stack | ||
| .pop() | ||
| .ok_or_else(CodeGenError::missing_values_in_stack)?; |
There was a problem hiding this comment.
I'd suggest materializing these values to registers via pop_to_reg instead of popping the raw value here: if these values are memory addresses, the push that happens below has the potential to break the stack ordering invariant if any spills happen during self.emit_compute_heap_address_align_checked. Meaning that values will no longer be stored from oldest to newest.
There was a problem hiding this comment.
i didn't think about that. It seems quite wasteful now that I think about it. If i understand correctly, rax will always be allocated by the time we pop expected: either by replacement or by a previous instruction. This means that xcmpchg will unconditionally spill all registers to the stack.
winch/codegen/src/isa/x64/masm.rs
Outdated
| if let Some(extend) = extend { | ||
| self.asm | ||
| .movzx_rr(expected.reg, writable!(expected.reg), extend); | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe we need to check if we actually need to make the zero extension? Similar to all the other cases?
if !(extend.from_bits() == 32 && extend.to_bits() == 64) {..}
MarinPostma
force-pushed
the
winch-x64-rmw-cmpxchg
branch
2 times, most recently
from
c9ec0de to
564d774
Compare
|
a test is failing with the newly enabled |
|
@saulecabrera we need to merge this one first: #10060 |
MarinPostma
force-pushed
the
winch-x64-rmw-cmpxchg
branch
4 times, most recently
from
b931624 to
219b11e
Compare
MarinPostma
force-pushed
the
winch-x64-rmw-cmpxchg
branch
from
219b11e to
348299a
Compare
|
@saulecabrera fixed the tests here, but I ultimately had to disable |
Right, I forgot to include those in the issue. It should now be up to date. |
This PR implements atomic CAS operations for winch x64 backend:
i32.atomic.rmw8.cmpxchg_ui32.atomic.rmw16.cmpxchg_ui32.atomic.rmw.cmpxchgi64.atomic.rmw8.cmpxchg_ui64.atomic.rmw16.cmpxchg_ui64.atomic.rmw32.cmpxchg_ui64.atomic.rmw.cmpxchg#9734