Stream: git-wasmtime

Topic: wasmtime / PR #9495 Improve `atomic_rmw` lowering on `x86`

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 16:17):

beetrees opened PR #9495 from beetrees:x64-better-atomics to bytecodealliance:main:

xchg and lock xadd can be used in all cases for Xchg, Add and Sub, while various lock-prefixed arithmetic instructions can be used for Add, Sub, And, Or and Xor if the result value of atomic_rmw (the old value) isn't used.

Closes #2153

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 16:17):

beetrees requested abrown for a review on PR #9495.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 16:17):

beetrees requested wasmtime-compiler-reviewers for a review on PR #9495.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:16):

abrown submitted PR review:

Thanks for the PR! This looks fine to me but I'm going to ask @cfallin to double-check a couple spots below.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:16):

abrown created PR review comment:

I believe this is correct but @cfallin do you want to double-check?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:16):

abrown created PR review comment:

Another note for @cfallin to check: this does in fact do what we would expect it to?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

We try to avoid any .. here so we don't miss new fields being added. Could we instead do lock: _?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

Now that we don't use all MachAtomicRmwOp options in lowerings anymore, could we define a separate enum for x64 and translate into it at the rule-match step, so we don't have this problem of representable but invalid combinations?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

Likewise here

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

Looks right to me!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

Likewise here

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 18:39):

cfallin created PR review comment:

Yes, I think so! We're doing a backward pass so we know for sure if a value is used or not, and this helper looks like it was added to be used for a similar purpose in a few other backends; perfectly valid to use it here to know if we can avoid producing the result.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 19:55):

beetrees updated PR #9495.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 19:59):

beetrees submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2024 at 19:59):

beetrees created PR review comment:

I've replaced MachAtomicRmwOp with the more specific AtomicRmwSeqOp and Atomic128RmwSeqOp. I then deleted MachAtomicRmwOp as it was now unused and essentially a duplicate of AtomicRmwOp.

Last updated: Oct 23 2024 at 20:03 UTC