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

[Sameer Sahasrabuddhe 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``.
----------------
ssahasra wrote:

> [@arsenm](https://github.com/arsenm) If we leave the specification of `syncscope` up to each target, afaict it would no longer be possible to optimize any atomic operation in the middle-end, 

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

> That is, I think for `syncscope` to be useful, the LangRef must precisely specify its semantics.

Sure, but that seems out of scope for this PR. The current text in the LangRef tries to say that `syncscope` has target-defined semantics, but fails to cover all the places. This PR merely adds what should have been added when `syncscope` was first introduced a long time ago.

> I think the spirit of `synscope` is to extend the definition of data-race as follows:
> 
> > The execution of a program contains a data race if it contains two potentially concurrent conflicting actions, **at least one of which is not atomic at a scope that includes the thread that performed the other operation**, and neither happens before the other, except for the special case for signal handlers described below. Any such data race results in undefined behavior. […]

That seems correct. But where is this quoted from?

> A minimal change in the style of the current LangRef (i.e. not super precise or clean) may be:
> 
> > Otherwise, if R is atomic, and all the writes R\ :sub:`byte` may see are atomic,
> > **and the `synscope`s of R and of these writes include all threads that issued these operations**,
> > then R chooses one of the values written.

That works for AMDGPU. But we cannot simply change it for all LLVM targets without a buy-in. 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.

[1] https://discourse.llvm.org/t/rfc-a-vulkan-style-memory-model-for-amdgpu-and-beyond/90498

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