<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/189660>189660</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            [PowerPC] compare_exchange_weak/strong don't fully respect seq_cst failure order
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
            new issue
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          pcordes
      </td>
    </tr>
</table>

<pre>
    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, unless we're doing CAS_strong and it has to retry for spurious failure.

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)`.

```
// Godbolt's install of powerpc64 Clang doesn't have an <atomic> 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.)

```
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?

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.
</pre>

<img width="1" height="1" alt="" src="http://email.email.llvm.org/o/eJykWFtv6zYS_jXMyyCCQvkSP-TBduoiQHd7ySn6aFDUyGJDkwpJWUkf9rcvhpRkO8npotjg4NimyLl8c-F8Et6rg0F8YPMNmz_eiC401j200roK_U1pq_eHJ6isYXwZ4MXYHgRoG0CUtgvwi-3R_bIFb-Eo3kuEJ8aXRzgq35kKnQ_CVMocwNsjhkaZA-NbKLsAT0A_X6BHaMQJQRnf1bWSCk2AGo2kU7V1EBoEKTxC36BD2K6f9z2KF6iF0h6MNbe-7ZyyndfvJNy6uMcHZ0nb0oO0x1Y4HE5YE0XWyvkAKqATQZF320FBj2CNfgc8oYPKgoC2cwjaiioDeArgG9vpCkoE4SGpoW_DPrbIaSvj9x5f99IHxldskWcALF9vukDyTZTtOgOC9vt3I9kiB8bvxUtcafpprXiEBsXpvUd1aALQelysO62hFM4pdIyvoMTaOoTQiADJgG3ygwyaxIk60PYtdEaj99Aj40uHUFmC-4wbCFOBCtAID8GCw-DeYzRGrCOYncOM5WuWr_9t3VFo_Q4CBrejEeDwtVMO_aWbyVIyQpABWtNn6YSRDeMbtsjVlbWEHL__7MUYB6G9pWAMhqOx3aHJCBIyjd83IbSeFWvGd4zv-r7PpM6kOGZCZt0L47v_tOh5zvhOtm36P387irZV5uCzJhw146sM4FujPCgPPSHcE2QRELbI93sR7FHJPbm8N4zftyGCvN-vv_38r6ft_vmHX_fb529DKiTI2CIf_uWDbfCjrUqrQ0xaZXwQWoOtoaUaa-ViBlstzAEqiz6VY6wcYYAV22QCK36gdKkIHgtP0HmsoOyUDsr4QXFprYbK7qXwe-8Zv-9MbAEVKBOA8cVbSpDrRXxrUQasPj2r0CtH6ytgyw3L1wBACdM5AxMyQwnu8U02whwwojSq-iB9EriFWmiPENFZG0uwixfGE1zbQRXAFzjHh1_BX5CFbPn4MQD5emuPrdJYQa9CM_a1CXNeZHmWHxRhcR9cZ16okUwBW8F1mh3Sg8y6A-O7vxjfLWff3pa-XvyRsnLMptQ08YQGpEbh0CX90dPiMbgOKZIqfIx6sKCtbcfEFP5I4igjh6MJO2oo57Kmsvicfd_LBcYXH6P95dKK3M7XoPu_gPG5W9KWPAZ47mYEzvjHeJH6wqesmmIJuhfuLcpZDHK28VcxSWgRKU5fdHCSfYtvUndenZBqZ0rpswJ5bHV_VpAsPj8uDd4mTXPpovbsp83mbj8btKNw-v2W7j5VX10_Y0ukxPho21l8bOBf_JHoy54-NnQfrMOzT7Ftxvb3wVdVgw-9fAPfSYlYfXHRsXydXOFDwEaT6FwWgZh_hJzMktZUitwQ-jaaMwI7VCqVBJ1cUVKOcS9Sqk2o4ivjm69wnQ_Ol45udcLV1tFwSm9y4gMCyUEfu9n_zJl_EPbvGsgHA9MlqOppnvBBaT3hzYoNGBtHF4ztW1UY_bi4VZOIWLdTMGbXwZDuzTqYMb6WhOmGusv2739f4nCZXmS0kK-M7xzqZBioISmGVB36dCtCc4nF4PliMnJ-beSVnr9RM4TqUk125Su-nv6pr6NpVwaVEu74V4fT7vvrbL_KlHOXIkdSfpPtXVuJgF-2qlHoNSZqSDm-hTtaL6V237Eq_865y_VSu8-XFMvXP5urgbZLoxRZ7FDjSZgQZwfXSSrZYcQ7YRo3HcKfnQ9pKBTuLY2dm6shMY3CSjZ0ocTdwsB6--vvT7_9MI2WdBsP12oG8HuaJlPVCpqNaZw8l2zr6IYLfqjl2rpeuMgLamePYEMTG9pBSaFBWhoYrUvLvfIIR_GCHp5-e_oDHBIxcXRWHVvrvSo1nk0eaYXyAyTCUzu2Dgxi0njRZKlOyQQTm85zukA8K3YJ6guOIoVJWBztKeXHxUQbbFwJ1K9S76o7E-Gn1mjdRccmC4Q7eNoobec8pqnk6xu-RCgx9IipnFKbo9F8aNmwSdcQTf1xakzUrMRA-kb2NFYhsSjK92JHOvl2GD4u-0EkWjLO1ZQvFz6S1tCgGXrYGOrBhvhEhauZ3GGVcBnSNTKSKaT9qKe1AU1QkT0MfSJOQBdtk6yLc2wAg_1-rETiQUSFrK6mtQzgmbSm6ap1thRlpCWlqEBVKMiKOG7VA1GMZDJRPxWgT9xOWxEG26uR47B8_f9zCTioE1KcHErVYuJUA2Parp_PhRdpxo_bbcwKVqwjfzxTw2IDe7pH6Emq5GJDVxx9lBL2-KYCfU-JMq6mExtQk4y4b5GP0q852sX8EMuaCKqpJq62i2JGTrmCowiyQQ-N7amWJtrsobfuJRtrSnjfHfHM-JIU5cF3bWuJsKTMlw2KFimFhblmscrERPVT2GLun98fZHQrXR5g_F4Tff5Ioktt5Utk-u_x5QS1LGwDPFOb-ikR2LHdkIdTV5liN1z3XUtGr-Xrb6jjvO5TDVy800g9QKSquakeimpVrMQNPtwtl7PVopjPipvmQfJ6OS_lXBZ3q2JW413J-XxZytndUpSzWtyoB57zRV4Ud3cFn8-KTNZ3iyXnOV-KZbWQNZvleBRKZ1qfjkRAbpT3HT7c3a8Wi_xGixK1j297ODfYQ3zKOGfzxxv3QIduy-7g2SzXygd_FhNU0PE10cCN2PwRPvG6xNB2Q-GOr42o51J1eyrSKd-neZkQvumcfvhAolRoujKT9sj4jswYPm5bZ_9EGWIG-g4947vBu9MD_28AAAD__4Dp_TM">