llvm / llvm/llvm-project

[PowerPC] compare_exchange_weak/strong don't fully respect seq_cst failure order

Open
#189,660 2 comments 0 reactions 0 assignees View on GitHub
backend:PowerPC miscompilation
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.