[acqrel] Add Turboshaft support for relaxed atomics [v8/v8 : main]
0 views
Skip to first unread message
Rezvan Mahdavi Hezaveh (Gerrit)
Feb 23, 2026, 7:49:23 PM (13 hours ago) Feb 23
to Jakob Kummerow, V8 LUCI CQ, dmercadi...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Jakob Kummerow
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 23 (Latest):
Rezvan Mahdavi Hezaveh . resolved
This CL is ready for review! PTAL, thanks.
(The bot timeouts happen on other CLs as well, so it is unrelated and should be fixed before landing the CL)
Related details
Attention is currently required from:
- Jakob Kummerow
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
6:29 AM (3 hours ago) 6:29 AM
to Rezvan Mahdavi Hezaveh, Jakob Kummerow, V8 LUCI CQ, dmercadi...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Rezvan Mahdavi Hezaveh
Jakob Kummerow voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
| Commit-Queue | +1 |
3 comments
Patchset-level comments
Jakob Kummerow . resolved
LGTM with a suggestion.
The bot timeouts tend to happen on all CLs when America is awake; it helps to start dry runs during the European working day 😊
File src/compiler/backend/instruction-selector.h
Line 905, Patchset 23 (Latest):
if (op_->memory_order_ == AtomicMemoryOrder::kAcqRel) {
return AtomicMemoryOrder::kAcqRel;
}
return AtomicMemoryOrder::kSeqCst;Jakob Kummerow . unresolved
Could this be just `return op_->memory_order_;`?
File src/compiler/turboshaft/assembler.h
Line 3099, Patchset 23 (Latest):
void StoreOffHeap(OpIndex address, OptionalOpIndex index, OpIndex value,Jakob Kummerow . unresolved
Similar to this precedent, I think if you added an `AtomicStore(...)` helper here that takes a `std::optional<AtomicMemoryOrder>`, and in turn left both existing `Store()` as they were, that could probably cut the size of this CL in half, because all the places where you're just adding a `std::nullopt` parameter could just stay as they are. WDYT?
Related details
Attention is currently required from:
- Rezvan Mahdavi Hezaveh
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages