Repository navigation
Behavior of t.atomic.rmw.cmpxchg instructions #195
Description
Hi,
As the 17. and 18. steps in the spec:
17. If c1 equals c2, then:
Let c be c2.
18. Else:
Let c be c3.
And the 21. step will write the c back to the memory.
Where c1 is the origin value on the memory, c2 is the expected value, c3 is the value to replace.
Maybe I can realize this as following simply:
if (memory[addr] != expected)
memory[addr] = replacement;
But the closest function in C++ std::atomic<T>::compare_exchange_strong has the different behavior.
if (memory[addr] == expected)
memory[addr] = replacement;
The atomic_rmw instructions are similar to the member functions of std::atomic<T> in C++ STL, such as fetch_add(), fetch_xor, etc.
Is it necessary to have the same behavior between cmpxchg instructions and std::atomic<T>::compare_exchange_strong in C++?
Thanks.
Activity
conrad-watt commentedon Nov 17, 2022
Ah, there are unfortunate mistakes in the draft spec text here. The overview correctly describes the intended behaviour, which is the same as the C++
compare_exchange_strong.q82419 commentedon Nov 17, 2022
Hi @conrad-watt ,
Thanks for the information!
Another question is that according to the overview:
But take the test suite for example:
When invoking the
i32.atomic.rmw8.cmpxchg_uin line 326, theaddressis0, the expected value is0x11, and the replacement value is0xcd. Therefore the loaded value is0x11.The loaded value
0x11is equal to the expected value0x11, so the replacement value0xcdis stored to the same memory address.Then when invoking the
i64.atomic.loadin line 327, the returned value shoud bei64.const 0x11111111111111cd, not0x1111111111111111?The same questions are at line 334, 342, 350, and 358.
Thanks.
conrad-watt commentedon Nov 17, 2022
Oh dear. You've identified an interesting discrepancy, and it looks like there is real implementation divergence here.
The culprit is the line in the overview which governs how value comparison works. It states that for
i32.atomic.rmw8.cmpxchg_u, the "expected" value is wrapped to 8 bits before it is compared. However the reference interpreter and test suite are acting as though the value is not wrapped before comparison (i.e. compare line 325 to line 371).Running the following test, it appears that Chrome and Firefox implement the overview semantics, while Safari implements the test suite semantics.
conrad-watt commentedon Nov 17, 2022
Well... now we have to work out which is the preferable semantics. Thanks for discovering this @q82419 :)
@dtig who would be the right person to ask regarding V8's code generation? Is the current V8 semantics implemented by wrapping the expected value to the packed width as a separate platform asm instruction, or is there an asm instruction that directly implements sub-32-bit
cmpxchgwith the wrapping semantics for the expected value?conrad-watt commentedon Nov 17, 2022
@Constellation it looks like WebKit's behaviour is the odd one out here, although the fault lies with the reference interpreter for implementing this behaviour in the first place (as seen in the tests added here, which pass in the reference interpreter but should fail according to the overview). In principle, would it be possible to change the behaviour here to align with V8 and SpiderMonkey? (we would also change the reference interpreter and relevant tests).
q82419 commentedon Nov 17, 2022
@conrad-watt
Thanks for your verification very much!
[Test] Fix the `atomic` tests in `threads` proposal.
[Test] Fix the `atomic` tests in `threads` proposal.
[Test] Add the threads tests.
[Test] Add the threads tests.
conrad-watt commentedon Oct 1, 2023
To bump this: the spec, interpreter, and tests in upstream-rebuild, and the spec render at https://webassembly.github.io/threads/ (see #198), should now reflect the following:
cmpxchgperforms the replacement if the expected value matches the read value (see here)cmpxchgwraps before comparing, enshrining the overview/Chrome/Firefox semantics (see here)Please let me know if any remaining artefacts fail to reflect the above. I'll flag these issues up again at the in-person meeting, since there really was an unfortunate divergence here.
Update wasm threads proposal tests and fix cmpxchg_u
Update wasm threads proposal tests and fix cmpxchg_u
[Test] Add the threads tests.
[Test] Add the threads tests.
[Test] Add the threads tests.
[Test] Add the threads tests.
[Test] Add the threads tests.
[Test] Add the threads tests.
[Test] Add the threads tests.