[mlir][OpenACC][OpenMP] Modify atomic capture to allow update/write #167963

erichkeane · 2025-11-13T21:51:32Z

OpenACC's C++ version has a variant of capture that permits a update followed by a write. Therefore the verifier was overly strict in this case.

According to our reading of the OpenMP 6.0 spec, it appears that atomic captured update (page 495) also requires this form, so it seems reasonable to allow this for both languages.

OpenACC's C++ version has a variant of capture that permits a update followed by a write. Therefore the verifier was overly strict in this case. According to our reading of the OpenMP 6.0 spec, it appears that `atomic captured update` (page 495) also requires this form, so it seems reasonable to allow this for both languages.

erichkeane · 2025-11-13T21:51:51Z

@razvanlupusoru : can you suggest OMP reviewers for me?

llvmbot · 2025-11-13T21:52:02Z

@llvm/pr-subscribers-mlir-openacc
@llvm/pr-subscribers-mlir-openmp

@llvm/pr-subscribers-mlir

Author: Erich Keane (erichkeane)

Changes

OpenACC's C++ version has a variant of capture that permits a update followed by a write. Therefore the verifier was overly strict in this case.

According to our reading of the OpenMP 6.0 spec, it appears that atomic captured update (page 495) also requires this form, so it seems reasonable to allow this for both languages.

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

4 Files Affected:

(modified) mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td (+6)
(modified) mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td (+6-2)
(modified) mlir/test/Dialect/OpenACC/invalid.mlir (+17-1)
(modified) mlir/test/Dialect/OpenMP/invalid.mlir (+32-1)

diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 5b89f741e296d..8c33be7ff1747 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2999,6 +2999,12 @@ def AtomicCaptureOp : OpenACC_Op<"atomic.capture",
         acc.atomic.write ...
         acc.terminator
       }
+
+      acc.atomic.capture {
+        acc.atomic.update ...
+        acc.atomic.write ...
+        acc.terminator
+      }
     ```
 
   }];
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
index 223bee9ab1c27..9df6b907eb326 100644
--- a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
@@ -239,6 +239,7 @@ def AtomicCaptureOpInterface : OpInterface<"AtomicCaptureOpInterface"> {
     implement one of the atomic interfaces. It can be found in one of these
     forms:
       `{ atomic.update, atomic.read }`
+      `{ atomic.update, atomic.write }`
       `{ atomic.read, atomic.update }`
       `{ atomic.read, atomic.write }`
   }];
@@ -291,12 +292,15 @@ def AtomicCaptureOpInterface : OpInterface<"AtomicCaptureOpInterface"> {
         auto secondWriteStmt = dyn_cast<AtomicWriteOpInterface>(secondOp);
 
         if (!((firstUpdateStmt && secondReadStmt) ||
+              (firstUpdateStmt && secondWriteStmt) ||
               (firstReadStmt && secondUpdateStmt) ||
               (firstReadStmt && secondWriteStmt)))
           return ops.front().emitError()
                 << "invalid sequence of operations in the capture region";
-        if (firstUpdateStmt && secondReadStmt &&
-            firstUpdateStmt.getX() != secondReadStmt.getX())
+        if ((firstUpdateStmt && secondReadStmt &&
+             firstUpdateStmt.getX() != secondReadStmt.getX()) ||
+            (firstUpdateStmt && secondWriteStmt &&
+             firstUpdateStmt.getX() != secondWriteStmt.getX()))
           return firstUpdateStmt.emitError()
                 << "updated variable in atomic.update must be captured in "
                     "second operation";
diff --git a/mlir/test/Dialect/OpenACC/invalid.mlir b/mlir/test/Dialect/OpenACC/invalid.mlir
index 0e75894eaeceb..173c6a74545f8 100644
--- a/mlir/test/Dialect/OpenACC/invalid.mlir
+++ b/mlir/test/Dialect/OpenACC/invalid.mlir
@@ -690,7 +690,6 @@ func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
 
 func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
   acc.atomic.capture {
-    // expected-error @below {{invalid sequence of operations in the capture region}}
     acc.atomic.update %x : memref<i32> {
     ^bb0(%xval: i32):
       %newval = llvm.add %xval, %expr : i32
@@ -704,6 +703,23 @@ func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
 
 // -----
 
+func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
+  acc.atomic.capture {
+    // expected-error @below {{updated variable in atomic.update must be captured in second operation}}
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+    acc.atomic.write %v = %expr : memref<i32>, i32
+
+    acc.terminator
+  }
+  return
+}
+
+// -----
+
 func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
   acc.atomic.capture {
     // expected-error @below {{invalid sequence of operations in the capture region}}
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index af24d969064ab..6224a236e7c78 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -1263,7 +1263,22 @@ func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
 
 func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
   omp.atomic.capture {
-    // expected-error @below {{invalid sequence of operations in the capture region}}
+    // expected-error @below {{updated variable in atomic.update must be captured in second operation}}
+    omp.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      omp.yield (%newval : i32)
+    }
+    omp.atomic.write %v = %expr : memref<i32>, i32
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
+  omp.atomic.capture {
     omp.atomic.update %x : memref<i32> {
     ^bb0(%xval: i32):
       %newval = llvm.add %xval, %expr : i32
@@ -1289,6 +1304,22 @@ func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
 
 // -----
 
+func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
+  omp.atomic.capture {
+    omp.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      omp.yield (%newval : i32)
+    }
+    omp.atomic.write %x = %expr : memref<i32>, i32
+
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
 func.func @omp_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>, %expr: i32) {
   omp.atomic.capture {
     // expected-error @below {{updated variable in atomic.update must be captured in second operation}}

razvanlupusoru · 2025-11-13T21:59:57Z

mlir/test/Dialect/OpenMP/invalid.mlir


 // -----

+func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {


Is this test used?

This is supposed to be a 'valid' config test, so I don't have diagnostics. That said, I seem to have double-added this 'is valid' test.

Can you add the "valid" tests to ops.mlir (mlir/test/Dialect/OpenACC/ops.mlir and mlir/test/Dialect/OpenMP/ops.mlir)?

The invalid tests look great to me.

Ah! Yes, I did that. Thanks for the help on where to put those!

razvanlupusoru

Looks great to me! Can you please allow a bit of time for the omp reviewers to take a look also? Thank you for this!

erichkeane · 2025-11-14T19:42:20Z

Looks great to me! Can you please allow a bit of time for the omp reviewers to take a look also? Thank you for this!

Thanks for the reviews! I intend to hold off until Monday on merging this (unless interest is shown in delaying further by others), so hopefully the OMP folks have a chance to take a look by then.

erichkeane · 2025-11-17T16:07:59Z

@razvanlupusoru :
I'm actually second guessing the standard here for OpenACC (for the second I decided was update/write). For example:

v = x binop= expr , or v = x = x binop expr.

In these cases it isn't clear to me WHICH are the correct x and v values.

write should be x = expr. Anything on the RHS of the elements for the single-op version are clearly updates, as they are effectively copy/pasted from there.

However, there is NOTHING that fits the format of v = expr for read/write/update.

There isn't an obvious relationship between the x values of the write and update according to those two. It would seem silly for it to be:

x = x binop= expr.

I ACTUALLY think what is happening here, is that this is a update/read, but I was confused, so I think I want to abandon this patch, right? (at least for OpenACC).

The idea is that the RHS of each of these is properly done, then the result is assigned to v (the LHS).

So unless you say otherwise, I think I want to close this?

kparzysz · 2025-11-17T16:20:51Z

it appears that atomic captured update (page 495) also requires this form

In OpenMP a capture is a read operation (capturing the value of the atomic variable in a non-atomic variable). Hence, atomic update can only contain a combination of {update, write} and read (in any order).

There is no atomic operation in OpenMP that would allow two writes to the atomic variable.

With that in mind, if OpenACC allows it, the MLIR verifier should allow it as well.

erichkeane · 2025-11-17T17:34:11Z

it appears that atomic captured update (page 495) also requires this form

In OpenMP a capture is a read operation (capturing the value of the atomic variable in a non-atomic variable). Hence, atomic update can only contain a combination of {update, write} and read (in any order).

There is no atomic operation in OpenMP that would allow two writes to the atomic variable.

With that in mind, if OpenACC allows it, the MLIR verifier should allow it as well.

I think it is supposed to be an update + read in the cases I was concerned about in OpenACC as well, so once Razvan confirms I'll likely close this PR without merging.

razvanlupusoru · 2025-11-17T20:49:16Z

I think it is supposed to be an update + read in the cases I was concerned about in OpenACC as well

OK - I agree - the v = x is a read not a write. I also misunderstood this when I reviewed your initial proposal. I was hyper-focused on the fact that in Fortran we did not have x++ so I concluded that there was a chance we did not have complete support in the representation. Thank you for taking a deeper look.

erichkeane · 2025-11-18T04:10:11Z

I think it is supposed to be an update + read in the cases I was concerned about in OpenACC as well

OK - I agree - the v = x is a read not a write. I also misunderstood this when I reviewed your initial proposal. I was hyper-focused on the fact that in Fortran we did not have x++ so I concluded that there was a chance we did not have complete support in the representation. Thank you for taking a deeper look.

Ok, thanks for confirming! I was messing around with it a bunch today and convinced myself I was wrong when I asked about it, and figured I must have talked you into it :)

I was so focused on the other components/how weird it looked, I got distracted. Fortunately while writing my tests to use it today I realized it was strange that there wasn't a 2-statement form of it, until it hit me :D

erichkeane requested a review from razvanlupusoru November 13, 2025 21:51

llvmbot added mlir mlir:openacc mlir:openmp openacc labels Nov 13, 2025

razvanlupusoru requested review from kparzysz and tblah November 13, 2025 21:58

razvanlupusoru reviewed Nov 13, 2025

View reviewed changes

erichkeane added 2 commits November 13, 2025 14:05

duplicate test removal

814de97

move valid tests to ops.mlir files

c5eee32

razvanlupusoru approved these changes Nov 13, 2025

View reviewed changes

erichkeane closed this Nov 17, 2025


		// -----

		func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {

[mlir][OpenACC][OpenMP] Modify atomic capture to allow update/write #167963

[mlir][OpenACC][OpenMP] Modify atomic capture to allow update/write #167963

Uh oh!

Conversation

erichkeane commented Nov 13, 2025

Uh oh!

erichkeane commented Nov 13, 2025

Uh oh!

llvmbot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

razvanlupusoru Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

razvanlupusoru Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

erichkeane Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

razvanlupusoru left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Nov 14, 2025

Uh oh!

erichkeane commented Nov 17, 2025

Uh oh!

kparzysz commented Nov 17, 2025

Uh oh!

erichkeane commented Nov 17, 2025

Uh oh!

razvanlupusoru commented Nov 17, 2025

Uh oh!

erichkeane commented Nov 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Nov 13, 2025 •

edited

Loading