[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

ilinpv added a comment. @sebpop could you ellaborate on __sync_* operations usage, are you getting issues with current Clang implementation? Do Clang need to keep supporting them and fix introducing new memory model? It seems we need compelling reasons to do that. >> However if sync

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. In D129802#3654943 , @Wilco1 wrote: > The general requirement is that inline and outline atomics have identical > behaviour, and that GCC and LLVM emit the same sequences. I agree sync is > badly documented, so it's hard to

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

asb added a comment. In D129802#3653544 , @efriedma wrote: > If we do decide we need to do something here, I'd prefer to model this using > explicit fences in the IR, as opposed to a new atomic ordering. Adding extra > atomic orderings just makes

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

Wilco1 added a comment. The general requirement is that inline and outline atomics have identical behaviour, and that GCC and LLVM emit the same sequences. I agree __sync is badly documented, so it's hard to figure whether an extra DMB barrier could actually make a difference, but it's best to

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

efriedma added a comment. `atomicrmw add [...] sync_seq_cst` is supposed to be equivalent to `fence seq_cst; atomicrmw add seq_cst; fence seq_cst`, I think. The extra fences aren't necessary for most uses of `__sync_*`, but the difference is theoretically visible for exotic synchronization

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697 is the relevant discussion thread that added these to GCC. I personally never found it very convincing, even back in 2015 when they first made the change. And, now, 7 years later, I'd be even more reluctant to add

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jrtc27 added a comment. I can't obviously see a description of what the additional barriers implied by the sync variants is (which should be in an update to LangRef at the very least, if not also in the summary itself). Inferring it from the AArch64 assembly is also difficult, and the RISC-V