[acqrel] Added atomic acquire load and release store instructions [v8/v8 : main]
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 11 comments![]( "Open in Gerrit")
Jakob and Deepti, thanks for your first round of comments! I would appreciate another round of early feedback. The architecture related `inl.h` files may need more work. You can ignore them for now or take them with a grain of salt.
__ AtomicStoreRelease(addr, index, offset, value, type, nullptr,Rezvan Mahdavi HezavehThis is the only difference from the existing `AtomicStoreMem`, right? That's not worth duplicating the whole function for. So I'd suggest to:
- drop `AtomicStoreReleaseMem`
- add the `order` parameter to `AtomicStoreMem`
- replace line 5940 with:
```
if (order == …::kAcqRel) {
__ AtomicStoreRelease(…);
} else {
__ AtomicStore(…);
}
```
Yeah that makes sense, done.
// I'll keep this one for now but we may remove it altogetherRezvan Mahdavi Hezaveh+1, similar to Store above.
Done
#define ATOMIC_STORE_RELEASE_LIST(V) \Rezvan Mahdavi HezavehIf you do the method merging I suggested above, then you can just add the new instructions to the old macro here (`ATOMIC_STORE_LIST`).
I removed the new instructions.
// x86 stores have release semantics. A simple mov is atomic for aligned
// accesses.Rezvan Mahdavi HezavehWe don't have any alignment guarantees here though. (There's an "alignment hint" in the `MemoryAccessImmediate`, but it's just that: a hint, not a guarantee, so entirely useless for codegen purposes.)
Ah, I see. That is a very good point. I think the alignment is checked in the caller of this function inside `liftoff-compiler.cc`. I removed the comment for more clarity.
if (this->enabled_.has_acquire_release()) { \
MemoryOrderImmediate memory_order(this, this->pc_ + opcode_length, \Rezvan Mahdavi HezavehThis isn't going to work: it adds a new immediate to the existing instructions when the V8 feature flag is enabled.
Instead, have a look at `MemoryAccessImmediate::ConstructSlow`, and add a new `if (alignment & 0x20)` block to it, similar to the existing handling of `alignment & 0x40`.
We'll then still want a feature check: if that `0x20` bit is set but the feature flag is off, fail validation.
Got it, thanks for the hint. Done.
AtomicMemoryOrder::kSeqCst; // set to the default valueRezvan Mahdavi Hezavehnit: All comments should be proper sentences, with capitalization at the beginning and punctuation at the end. See https://google.github.io/styleguide/cppguide.html#Comments.
Done
#include "src/flags/flags.h"Rezvan Mahdavi HezavehThis should be alphabetically sorted with the other includes, i.e. between lines 24/25. (Or just drop it and rely on transitive includes... that's arguably not super clean but it's the status quo.)
Pro tip: add `-header-insertion=never` to your clangd arguments and deal with headers manually, to avoid such misplaced auto insertions. See https://v8.dev/docs/ide-setup.
Great tip, thanks!
V(I32AtomicLoadAcquire, 0xfe50, i_i, "i32.atomic.load_acq", i_l) \
V(I64AtomicLoadAcquire, 0xfe51, l_i, "i64.atomic.load_acq", l_l) \
V(I32AtomicStoreRelease, 0xfe52, v_ii, "i32.atomic.store_rel", v_li) \
V(I64AtomicStoreRelease, 0xfe53, v_il, "i64.atomic.store_rel", v_ll)Rezvan Mahdavi HezavehThis would technically work (at least for a prototype), but it's not what https://github.com/WebAssembly/shared-everything-threads/blob/main/proposals/shared-everything-threads/Overview.md#memory-orderings specifies. We should probably implement the proposal, unless there's a reason not to?
When I start the implementation, I added new instructions to make sure I am not breaking the existing ones while prototyping and understanding the code. My plan was to remove them and modify the code later on. Now that I spent more time on the code, I think it's easier if I go with the proposal approach instead of having new opcodes. Removed them.
kExprLocalGet,
0,Rezvan Mahdavi HezavehThis looks auto-formatted, which is technically acceptable, but for (slightly) increased readability, it makes more sense to keep each instruction on one line, e.g.:
```
kExprLocalGet, 0,
kAtomicPrefix, kExprI32AtomicLoad, 0x22, kAtomicAcqRel, 0,
```(Feel free to define a named constant or helper function for `0x22`; or perhaps we even want a builder for memory accesses that takes an options bag such as `MemoryAccess({alignment: ..., offset: ..., memIndex: ..., atomicOrder: ...})`, or perhaps any of these ideas should be a separate CL.)
Yes, is file is auto-formatted. I can see why this format decrease the readability, I fixed the format.
About the second part of your comment, let's do it in a separate CL.
var builder = new WasmModuleBuilder();Rezvan Mahdavi HezavehTo improve efficiency of the test, don't create new module builders unless necessary: one Wasm function per atomic instruction is fine, but they can all be added to the same module.
Usually, the one case where we do need multiple module builders is when we want to test validation failures: while a module can of course contain many invalid things, usually all tests will first run into the same problem, and report that as the failure reason.
(I can see that some of the existing tests use one module builder per tested instruction as well, so you've probably copied this pattern from there. Cleaning up the precedent would be nice but is optional.)
Got it. Yeah, there are some other tests with similar structure. I updated this test to have just one module.
Related details
- Deepti Gandluri
- Jakob Kummerow
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 8 comments![]( "Open in Gerrit")
Looking good, some minor comments. Didn't look at the `-inl.h` files.
bool kAcquireReleaseAtomicEnables = decoder->enabled_.has_acquire_release();In this function, we don't actually care about validation, see e.g. `WasmEnabledFeatures::All()` in line 2416. So this can be:
```
constexpr bool kAcqRelEnabled = true;
```
(Or consider following the `WasmEnabledFeatures::All()` precedent instead of passing that bool.)
"--experimental--acquire-release flag");`-wasm-`
if (!VALIDATE(this->enabled_.has_acquire_release() ||
imm.memory_order != AtomicMemoryOrder::kAcqRel)) {This can't ever happen, right? The condition on line 1123 makes it impossible.
I think the block starting in line 1091 provides all required validation. If you want, you can improve the error message there, something like:
```
if (alignment & 0x20) {
DecodeError("acqrel requires --experimental-wasm-acquire-release");
} else {
// existing DecodeError
}
```
Spec question: what should happen when a non-atomic load or store instruction's alignment immediate has the acqrel bit set?
// Copyright 2025 the V8 project authors. All rights reserved.nit: 2026 now 😊
(function TestAllInOneModule() {
print(arguments.callee.name);nit: just drop this (and the corresponding `}` at the end).
.exportAs('i32_atomic_load_acquire');nit: use `exportFunc()`, then you don't need to repeat the function name (and then you can save lines by putting it at the end of the `.addFunction(...)` line, before `.addBody(...)`).
assertEquals(0, exports.i32_atomic_load_acquire(0));If you did one `exports.i64_atomic_store_release(0, 0x1234567890abcdefn)` before, you'd have more interesting expectation values for the loads.
(Tests should, within reason, rule out as many bugs as they can: suppose some platform's assembler implementation accidentally read from the wrong address, or always returned zero; as currently written this test would still pass.)
Related details
- Deepti Gandluri
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Message from Rezvan Mahdavi Hezaveh![]( "Open in Gerrit")
- Deepti Gandluri
- Jakob Kummerow
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 9 comments![]( "Open in Gerrit")
This CL is ready for review. Thanks!
// x86 stores have release semantics. A simple mov is atomic for aligned
// accesses.Rezvan Mahdavi HezavehWe don't have any alignment guarantees here though. (There's an "alignment hint" in the `MemoryAccessImmediate`, but it's just that: a hint, not a guarantee, so entirely useless for codegen purposes.)
Ah, I see. That is a very good point. I think the alignment is checked in the caller of this function inside `liftoff-compiler.cc`. I removed the comment for more clarity.
It meant to be a question here: I think the alignment is checked in the caller of this function inside liftoff-compiler.cc. Right?
bool kAcquireReleaseAtomicEnables = decoder->enabled_.has_acquire_release();In this function, we don't actually care about validation, see e.g. `WasmEnabledFeatures::All()` in line 2416. So this can be:
```
constexpr bool kAcqRelEnabled = true;
```
(Or consider following the `WasmEnabledFeatures::All()` precedent instead of passing that bool.)
Got it. I decided to go with the `true` value.
"--experimental--acquire-release flag");Rezvan Mahdavi Hezaveh`-wasm-`
Done
if (!VALIDATE(this->enabled_.has_acquire_release() ||
imm.memory_order != AtomicMemoryOrder::kAcqRel)) {This can't ever happen, right? The condition on line 1123 makes it impossible.
I think the block starting in line 1091 provides all required validation. If you want, you can improve the error message there, something like:
```
if (alignment & 0x20) {
DecodeError("acqrel requires --experimental-wasm-acquire-release");
} else {
// existing DecodeError
}
```Spec question: what should happen when a non-atomic load or store instruction's alignment immediate has the acqrel bit set?
Done.
About your question: My answer is that it should be a validation error. I added the check to the code and opened a PR to add this to the spec overview (https://github.com/WebAssembly/shared-everything-threads/pull/107). Thanks for the great question!
// Copyright 2025 the V8 project authors. All rights reserved.Rezvan Mahdavi Hezavehnit: 2026 now 😊
Oh yeah 😊
(function TestAllInOneModule() {
print(arguments.callee.name);nit: just drop this (and the corresponding `}` at the end).
mmm I am not sure if I understand this comment. I can't see extra/unnecessary `(` or `{`.
nit: use `exportFunc()`, then you don't need to repeat the function name (and then you can save lines by putting it at the end of the `.addFunction(...)` line, before `.addBody(...)`).
Done
assertEquals(0, exports.i32_atomic_load_acquire(0));If you did one `exports.i64_atomic_store_release(0, 0x1234567890abcdefn)` before, you'd have more interesting expectation values for the loads.
(Tests should, within reason, rule out as many bugs as they can: suppose some platform's assembler implementation accidentally read from the wrong address, or always returned zero; as currently written this test would still pass.)
That's a good point, sure!
I think we need some tests with shared memory as well to see of the instructions actually works and acquire and release semantics are correct. Can you provide some pointers for that? I know that one example is `test/mjsunit/wasm/shared-everything/atomic-instructions.js` but I still appreciate some pointers/hints. I may add these correctness tests in follow-up CLs.
Related details
- Deepti Gandluri
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
Oh wait. I got some compile errors on some architectures. Let me fix them and send the correct CL for review. Sorry for the previous ping.
Related details
- Deepti Gandluri
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |