[Mlir-commits] [mlir] [mlir][acc] Declare the memory effects of acc.atomic.read (PR #228899)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-openacc

Author: Kareem Ergawy (ergawy)

<details>
<summary>Changes</summary>

The operation read `x` and wrote `v` but declared no memory effects at all, so every memory analysis had to treat both locations as unknown and stay maximally conservative. In particular it could not be seen to overwrite `v`.

Declare the effects per operand so the read on `x` and the write on `v` are visible.

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


2 Files Affected:

- (modified) mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td (+2-2) 
- (added) mlir/test/Dialect/OpenACC/side-effects.mlir (+42) 


``````````diff
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index b7914a867c8d2..f405237a067b2 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -3044,8 +3044,8 @@ def AtomicReadOp : OpenACC_Op<"atomic.read", [AtomicReadOpInterface]> {
     The operand `v` is the address where the value is stored after reading.
   }];
 
-  let arguments = (ins OpenACC_PointerLikeType:$x,
-                       OpenACC_PointerLikeType:$v,
+  let arguments = (ins Arg<OpenACC_PointerLikeType, "", [MemRead]>:$x,
+                       Arg<OpenACC_PointerLikeType, "", [MemWrite]>:$v,
                        TypeAttr:$element_type, Optional<I1>:$ifCond);
   let assemblyFormat = [{
     oilist(
diff --git a/mlir/test/Dialect/OpenACC/side-effects.mlir b/mlir/test/Dialect/OpenACC/side-effects.mlir
new file mode 100644
index 0000000000000..1a7f98cefba8c
--- /dev/null
+++ b/mlir/test/Dialect/OpenACC/side-effects.mlir
@@ -0,0 +1,42 @@
+// RUN: mlir-opt %s --test-side-effects --verify-diagnostics --split-input-file
+
+// acc.atomic.read declares its effects per operand: it reads the location
+// designated by `x` and writes the location designated by `v`. Without them the
+// operation reports no effects at all, which every memory analysis has to read
+// as "unknown" and treat maximally conservatively -- in particular it cannot be
+// seen to overwrite `v`.
+
+func.func @atomic_read(%x: memref<i32>, %v: memref<i32>) {
+  // expected-remark @below {{found an instance of 'read' on op operand 0, on resource '<Default>'}}
+  // expected-remark @below {{found an instance of 'write' on op operand 1, on resource '<Default>'}}
+  acc.atomic.read %v = %x : memref<i32>, memref<i32>, i32
+  return
+}
+
+// -----
+
+// The same effects are reported inside an acc.atomic.capture region. The
+// capture itself carries RecursiveMemoryEffects and so does not implement
+// MemoryEffectOpInterface; the test pass walks only operations that do, so the
+// capture gets no remark of its own and the effects come from its body. The
+// read writes `v` and reads `x`; the update both reads and writes `x`, which is
+// what keeps `x` from being treated as overwritten.
+
+func.func @atomic_capture(%x: memref<i32>, %v: memref<i32>) {
+  acc.atomic.capture {
+    // expected-remark @below {{found an instance of 'read' on op operand 0, on resource '<Default>'}}
+    // expected-remark @below {{found an instance of 'write' on op operand 1, on resource '<Default>'}}
+    acc.atomic.read %v = %x : memref<i32>, memref<i32>, i32
+    // expected-remark @below {{found an instance of 'read' on op operand 0, on resource '<Default>'}}
+    // expected-remark @below {{found an instance of 'write' on op operand 0, on resource '<Default>'}}
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%arg0: i32):
+      // expected-remark @below {{operation has no memory effects}}
+      %0 = arith.constant 1 : i32
+      // expected-remark @below {{operation has no memory effects}}
+      %1 = arith.addi %arg0, %0 : i32
+      acc.yield %1 : i32
+    }
+  }
+  return
+}

``````````

</details>


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