[llvm-branch-commits] [llvm] [LangRef][AMDGPU] Specify that syncscope can cause atomic operations to race (PR #189015)

[via llvm-branch-commits]

================
@@ -3934,7 +3934,8 @@ Given that definition, R\ :sub:`byte` is defined as follows:
 -  Otherwise, if R is atomic, and all the writes R\ :sub:`byte` may
    see are atomic, it chooses one of the values written. See the :ref:`Atomic
    Memory Ordering Constraints <ordering>` section for additional
-   constraints on how the choice is made.
+   constraints on how the choice is made. Targets may impose additional
+   requirements on R and the writes it may see based on their ``syncscope``.
----------------
gonzalobg wrote:

> That is already true. Any scope other than "" and "singlethread" specified in the syncscope parameter has target-defined semantics. See the last paragraph here:
https://llvm.org/docs/LangRef.html#id2073

My reading of that section is that the group of threads is definetly target-specific. Its not clear to me that the synchronization behavior among threads of that group was also intended to be target specific despite that being what that section suggest. Was this clarified in some other discussion? (I found: [discussion0](https://lists.llvm.org/pipermail/llvm-dev/2015-January/080236.html), [discussion1](https://groups.google.com/g/llvm-dev/c/GtWfCc5j-4U)). I can imagine this being intentional, but if that is the case, that seems hard to reconcile with any IR pass that manipulates atomics being sound (that'd be like declaring that the behavior of atomics is target-specific), the clang `__scope`d atomic builtins intent to be target-agnostic, etc.

> That seems correct. But where is this quoted from?

[CUDA C++ Programming Guide - Memory Model](https://docs.nvidia.com/cuda/cuda-programming-guide/05-appendices/cuda-cpp-memory-model.html#data-races).

> In fact we are already trying to build something much more comprehensive in a separate RFC[1]. It will be really good to continue this discussion over there.

That discussion seems to be about a more radical change to the LLVM IR memory model, with changes spanning beyond just `syncscope`.  I'd prefer if we could separate the discussion about this aspect of `syncscope` from that discussion. I agree that this PR is not the right place to have it (and this discussion should not block it), maybe we can move it to a github issue?

The unspecified semantics of system scope is already tracked here: https://github.com/llvm/llvm-project/issues/123339 

https://github.com/llvm/llvm-project/pull/189015