AMDGPU: Add description for new atomicrmw metadata

[arsenm]

Add a spec for yet-to-be-implemented metadata to allow the backend to
fully handle atomicrmw lowering. This is the base of an alternative
to #69229, which inverts the direction to be correct by default, and
extends to cover the peer device case.

Could use a better name

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

Add a spec for yet-to-be-implemented metadata to allow the backend to
fully handle atomicrmw lowering. This is the base of an alternative
to #69229, which inverts the direction to be correct by default, and
extends to cover the peer device case.

Could use a better name

---

Full diff: https://github.com/llvm/llvm-project/pull/85052.diff

2 Files Affected:

• (modified) llvm/docs/AMDGPUUsage.rst (+61-12)
• (modified) llvm/docs/ReleaseNotes.rst (+2)

diff --git a/llvm/docs/AMDGPUUsage.rst b/llvm/docs/AMDGPUUsage.rst
index fd9ad7fac19a95..a6556bbd1752b9 100644
--- a/llvm/docs/AMDGPUUsage.rst
+++ b/llvm/docs/AMDGPUUsage.rst
@@ -1312,24 +1312,73 @@ The AMDGPU backend implements the following LLVM IR intrinsics.
 
    List AMDGPU intrinsics.
 
+.. _amdgpu_metadata:
+
 LLVM IR Metadata
-------------------
+================
+
+The AMDGPU backend implements the following target custom LLVM IR
+metadata.
+
+.. _amdgpu_last_use:
+
+'``amdgpu.last.use``' Metadata
+------------------------------
+
+Sets TH_LOAD_LU temporal hint on load instructions that support it.
+Takes priority over nontemporal hint (TH_LOAD_NT). This takes no
+arguments.
+
+.. code-block:: llvm
+
+  %val = load i32, ptr %in, align 4, !amdgpu.last.use !{}
 
-The AMDGPU backend implements the following LLVM IR metadata.
+.. _amdgpu_no_access_location_types:
 
-.. list-table:: AMDGPU LLVM IR Metatdata
-  :name: amdgpu-llvm-ir-metadata-table
+'``amdgpu.no.access.location.types``' Metadata
+----------------------------------------------
 
-  * - Metadata Name
+Asserts a memory access does not access bytes residing in certain
+allocation kinds. This is intended for use with :ref:`atomicrmw
+<i_atomicrmw>` and other atomic instructions. This is required to emit
+a native hardware instruction for some :ref:`system scope
+<amdgpu-memory-scopes>` atomic operations on some subtargets. An
+:ref:`atomicrmw <i_atomicrmw>` without metadata will be treated
+conservatively as required to preserve the operation behavior in all
+cases.
+
+If the memory operation does access an address in an indicated region,
+any stored values and any returned results are :ref:`poison
+<poisonvalues>`. This has a single integer argument, interpreted as a
+bitfield. A 0 value is equivalent to removing the metadata.
+
+.. list-table::
+
+  * - Bit
     - Description
-    - Values
-  * - !amdgpu.last.use
-    - Sets TH_LOAD_LU temporal hint on load instructions that support it.
-      Takes priority over nontemporal hint (TH_LOAD_NT).
-    - {}
+  * - 0
+    - Not in fine-grained host memory.
+  * - 1
+    - Not in a remote connected peer device (address must be device local)
+
+.. code-block:: llvm
+
+  ; Indicates the access does not access fine-grained memory, or
+  ; remote device memory.
+  %old0 = atomicrmw sub ptr %ptr0, i32 1 acquire, !amdgpu.no.access.location.types !0
+
+  ; Indicates the access does not access fine-grained memory.
+  %old1 = atomicrmw sub ptr %ptr1, i32 1 acquire, !amdgpu.no.access.location.types !1
+
+  ; Indicates the access does not access peer device memory.
+  %old2 = atomicrmw sub ptr %ptr2, i32 1 acquire, !amdgpu.no.access.location.types !2
+
+  !0 = !{i32 3}
+  !1 = !{i32 1}
+  !2 = !{i32 2}
 
 LLVM IR Attributes
-------------------
+==================
 
 The AMDGPU backend supports the following LLVM IR attributes.
 
@@ -1451,7 +1500,7 @@ The AMDGPU backend supports the following LLVM IR attributes.
      ======================================= ==========================================================
 
 Calling Conventions
--------------------
+===================
 
 The AMDGPU backend supports the following calling conventions:
 
diff --git a/llvm/docs/ReleaseNotes.rst b/llvm/docs/ReleaseNotes.rst
index b34a5f31c5eb0a..95ebbb74fbbd7f 100644
--- a/llvm/docs/ReleaseNotes.rst
+++ b/llvm/docs/ReleaseNotes.rst
@@ -71,6 +71,8 @@ Changes to the AMDGPU Backend
 -----------------------------
 
 * Implemented the ``llvm.get.fpenv`` and ``llvm.set.fpenv`` intrinsics.
+* Added ``!amdgpu.no.access.location.types`` metadata to control
+  atomic behavior.
 
 Changes to the ARM Backend
 --------------------------

[yxsamliu]

I feel using a bit that is negatively defined may cause confusing, especially when we try to turn it on/off in clang by pragmas.

#pragma clang atomic begin non_fine_grained(off) non_remote(off)
#pragma clang atomic end

I am wondering whether it is better to have positive bits:

- 0
- Maybe in fine-grained host memory
- 1
- Maybe in a remote connected peer device

Different bits are or-ed.

Then in clang

#pragma clang atomic begin maybe-fine-grained(off) maybe-remote(off)
#pragma clang atomic end

[arsenm]

Negatively expressing it is the less confusing direction. Every attribute specified positively has found a way to be a problem at some point

[pasaulais]

To make the name shorter, what about amdgpu.mem.constraint or amdgpu.access.constraint? Then the names of each constraint could be something like no_fine_grained_access and no_remote_access or similar.

[pasaulais]

For better clarity, I think it would help to have comments translating the bits to the actual values in documentation examples and lit tests. For example:

!0 = !{i32 3} ; no_fine_grained_access | no_remote_access

In general I think it would be more clear to use strings in the MD nodes (I am thinking of someone looking at the IR produced by the compiler and not knowing what the numbers mean without looking them up), but I am aware that would take away the simplicity of the current design.

[arsenm]

IR readability isn't really a priority, but if we're going to split out this into named pieces I think it's better to just use separate top level metadata, so the latest revision does that

[yxsamliu]

LGTM

[arsenm]

Wondering if both pieces should end in "memory" or "memory.access" for consistency

[yxsamliu]

Wondering if both pieces should end in "memory" or "memory.access" for consistency

I think "access" is unnecessary.

[b-sumner]

Wondering if both pieces should end in "memory" or "memory.access" for consistency

I think "access" is unnecessary.

I tend to agree.