Stream: git-wasmtime

Topic: wasmtime / PR #10039 Winch: x64 atomic CAS

Wasmtime GitHub notifications bot (Jan 17 2025 at 13:23):

MarinPostma requested fitzgen for a review on PR #10039.

Wasmtime GitHub notifications bot (Jan 17 2025 at 13:23):

MarinPostma opened PR #10039 from MarinPostma:winch-x64-rmw-cmpxchg to bytecodealliance:main:

This PR implements atomic CAS operations for winch x64 backend:

• i32.atomic.rmw8.cmpxchg_u
• i32.atomic.rmw16.cmpxchg_u
• i32.atomic.rmw.cmpxchg
• i64.atomic.rmw8.cmpxchg_u
• i64.atomic.rmw16.cmpxchg_u
• i64.atomic.rmw32.cmpxchg_u
• i64.atomic.rmw.cmpxchg

#9734

Wasmtime GitHub notifications bot (Jan 17 2025 at 13:23):

MarinPostma requested wasmtime-compiler-reviewers for a review on PR #10039.

Wasmtime GitHub notifications bot (Jan 17 2025 at 13:23):

MarinPostma requested wasmtime-core-reviewers for a review on PR #10039.

Wasmtime GitHub notifications bot (Jan 17 2025 at 13:33):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 17 2025 at 14:44):

github-actions[bot] commented on PR #10039:

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 17 2025 at 21:30):

saulecabrera commented on PR #10039:

I'll take a look at this one.

Wasmtime GitHub notifications bot (Jan 17 2025 at 21:31):

saulecabrera requested saulecabrera for a review on PR #10039.

Wasmtime GitHub notifications bot (Jan 19 2025 at 17:32):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 20 2025 at 18:55):

saulecabrera submitted PR review:

I believe we can now enable the remaining spec tests? https://github.com/bytecodealliance/wasmtime/blob/02ff25115176e04ceac14daf88166961a5fe6c91/crates/wast-util/src/lib.rs#L373

Wasmtime GitHub notifications bot (Jan 20 2025 at 18:55):

saulecabrera created PR review 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.

Wasmtime GitHub notifications bot (Jan 20 2025 at 18:55):

saulecabrera created PR review 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) {..}

Wasmtime GitHub notifications bot (Jan 20 2025 at 21:26):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 20 2025 at 21:40):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 20 2025 at 21:43):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 20 2025 at 21:59):

MarinPostma submitted PR review.

Wasmtime GitHub notifications bot (Jan 20 2025 at 21:59):

MarinPostma created PR review 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.

Wasmtime GitHub notifications bot (Jan 20 2025 at 22:40):

MarinPostma commented on PR #10039:

a test is failing with the newly enabled atomic.wast spec test. Looking into it.

Wasmtime GitHub notifications bot (Jan 21 2025 at 12:59):

MarinPostma commented on PR #10039:

@saulecabrera we need to merge this one first: https://github.com/bytecodealliance/wasmtime/pull/10060

Wasmtime GitHub notifications bot (Jan 21 2025 at 21:17):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:02):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:04):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:22):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:41):

MarinPostma updated PR #10039.

Wasmtime GitHub notifications bot (Jan 21 2025 at 23:58):

MarinPostma commented on PR #10039:

@saulecabrera fixed the tests here, but I ultimately had to disable atomic.wast, because we're missing notify

Wasmtime GitHub notifications bot (Jan 22 2025 at 01:29):

saulecabrera commented on PR #10039:

@saulecabrera fixed the tests here, but I ultimately had to disable atomic.wast, because we're missing notify

Right, I forgot to include those in the issue. It should now be up to date.

Wasmtime GitHub notifications bot (Jan 22 2025 at 01:29):

saulecabrera submitted PR review:

LGTM!

Wasmtime GitHub notifications bot (Jan 22 2025 at 01:52):

saulecabrera merged PR #10039.

Last updated: Jan 24 2025 at 00:11 UTC