dingcyrus wrote: > Sorry for sitting on this, my "changes-requested" was the only thing left > holding it. > > On the alignment question you asked back: leave it as it is, no explicit > alignment needed. `checkAtomicAlignment` is a faithful port of OG's, > force-to-natural path included: > > ```c++ > // Force address to be at least naturally-aligned. > return ptr.withAlignment(CharUnits::fromQuantity(bytes)); > ``` > > so classic lands on natural alignment for these builtins whatever the > pointer's declared alignment was. Setting it from `destAddr` would be the > thing that could diverge, not the default. Your test pins it either way now, > and since the third RUN line reuses the `LLVM` prefix over classic's output, > both columns have to produce the same `align N`. > > Two things in the description, which is what ends up in the log: > > * It still says `__sync_lock_test_and_set -> cir.atomic.xchg (acquire)`. > The code emits `seq_cst` now, which is right, `EmitBinaryAtomic` uses seq_cst > for it, but the summary did not follow the fix. > > * "covering 30 builtin variants" is 20 at this point, with the unsuffixed > forms now `llvm_unreachable` and `_16` back on `errorBuiltinNYI`.
Thanks! Description updated — __sync_lock_test_and_set now reads seq_cst, and the summary now says 20 size-suffixed variants, with a note that the unsuffixed forms are rewritten by Sema and _16 falls back to errorBuiltinNYI. https://github.com/llvm/llvm-project/pull/214606 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
