[PowerPC] compare_exchange_weak/strong don't fully respect seq_cst failure order
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
I don't know a lot about PowerPC so maybe I'm misunderstanding something, but I think we have insufficient fencing for the case where CAS_weak fails non-spuriously, or CAS_strong's compare fails on the first iteration, where we only ever do a pure load. It should be as strong as a pure `load(seq_cst)`.
But we never run a `sync` (aka `hwsync` = heavyweight sync = full barrier) before that load, only `lwsync` after, in those early-out non-spurious failure cases.
Normally a seq_cst load requires `sync` before, as well as branch+`isync` after. (`lwsync` after should also be strong enough.)
(https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html). This is what we do for `__atomic_load_n(ptr, __ATOMIC_SEQ_CST)`.
```c++
// Godbolt's install of powerpc64 Clang doesn't have an header so I used builtins.
bool do_cas_ss(unsigned int &x, unsigned int &expected, unsigned int desired) {
return __atomic_compare_exchange_n(&x, &expected, desired, false /*not weak*/,
__ATOMIC_SEQ_CST,
__ATOMIC_SEQ_CST);
}
```
Compiled with PowerPC64 Clang 23.0.0git (trunk on Godbolt) https://godbolt.org/z/74Tx7sf6W
(This is maybe even clearer with weak=true so it doesn't have to loop. This asm is for weak=false aka CAS_strong.)
```asm
do_cas_ss(unsigned int&, unsigned int&, unsigned int):
lwz %r7, 0(%r4) # load int &expected
lwarx %r6, 0, %r3 # peeled first iteration, load-exclusive of int &x
cmplw %r6, %r7
bne- %cr0, .LBB1_4 # early-out if non-spurious failure on first iteration
sync # full barrier before store-exclusive, after load-exclusive if stwcx succeeds on the first iter
.LBB1_2:
stwcx. %r5, 0, %r3 # conditional-store of int desired (r5) to 0(%r3)
beq+ %cr0, .LBB1_5 # break out of the loop on store-exclusive success.
lwarx %r6, 0, %r3
cmplw %r6, %r7
beq+ %cr0, .LBB1_2 # retry if compare still succeeds; no fences inside the CAS_strong retry loop.
.LBB1_4:
crxor 4*cr5+lt, 4*cr5+lt, 4*cr5+lt
lwsync # acq/rel fence in the failure return path
b .LBB1_6
.LBB1_5:
lwsync # acq/rel fence in the success return path.
creqv 4*cr5+lt, 4*cr5+lt, 4*cr5+lt
.LBB1_6:
bc 12, 4*cr5+lt, .LBB1_8
stw %r6, 0(%r4) # store the updated int &expected
.LBB1_8:
li %r3, 1
bclr 12, 4*cr5+lt, 0
li %r3, 0
blr
```
On compare failure, the relevant instructions we've run are just `lwarx` + `lwsync`, which is just an ACQUIRE load, not SEQ_CST. Unless the load being exclusive prevents store-forwarding from other logical cores or otherwise makes IRIW reordering impossible, which I think is the reason for needing full barriers in front of SC loads? But no, that's not plausible, we use the standard SC store recipe of just `sync; st` so SC loads need a fence to prevent StoreLoad reordering with previous stores from this thread.
I think we can just move the `sync` to the top of the function (or after loading args of course); it doesn't have to be between the lwarx and stwcx. But if that's a lot better for the success case(?), maybe the failure case could run `sync` and then retry the load. But then it should also redo the compare, otherwise we could potentially return with CAS_strong failed but new_expected == old_expected. So this is probably a bad idea, even for CAS_weak where it would bloat the code.
https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html gives a recipe for seq_cst CAS which is what GCC does:
`hwsync; _loop: lwarx; cmp; bc _exit; stwcx.; bc _loop; isync; _exit`
`sync` before the first load (and branch/isync after) matches how SC pure loads work.
I assume branch+isync is supposed to be cheaper than `lwsync` in cases where it's sufficient. `lwsync` (lightweight sync = block everything except StoreLoad reordering) is the recipe for fences up to AcqRel so is also sufficient after a load.
----
In a lot of code, the RELAXED is fine for the CAS failure order, in which case our current code-gen is a good optimization, if lwarx/stwcx being an atomic RMW is sufficiently strong to prevent StoreLoad reordering with previous SC stores to different locations. I'm not sure it is, that might be a separate bug in terms of respecting the success order!
---
Related about the failure-case load being required to work like a normal load with that memory-order:
* https://stackoverflow.com/questions/79913163/does-this-cas-operation-which-is-a-pure-load-with-a-seq-cst-memory-order-guara
* https://lists.isocpp.org/std-discussion/2026/03/3317.php discussion about the ISO standard's wording being "a bit ambiguous on the ordering semantics of spurious failure". (Not relevant to this bug, which is about non-spurious failure. Plus, I think everyone agrees that the intent is the failure load should be as strong as a normal pure load with the specified order.)
Contributor guide
Research direction
Start by reproducing the PowerPC64 Clang output for __atomic_compare_exchange_n shown in the issue, comparing weak and strong operations with seq_cst failure order. Trace the compare_exchange lowering and verify that non-spurious failure has the ordering of a seq_cst load, while relaxed failure remains optimized; done when the generated PowerPC instructions and relevant tests reflect the required ordering.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100