[Mlir-commits] [mlir] [openacc][openmp] Add dialect representation for acc atomic operations (PR #65493)

Razvan Lupusoru llvmlistbot at llvm.org
Wed Sep 6 10:03:40 PDT 2023


https://github.com/razvanlupusoru updated https://github.com/llvm/llvm-project/pull/65493:

>From 47525590e0bece1681d1db4c0b758f2ddc06fac4 Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Thu, 24 Aug 2023 13:03:51 -0700
Subject: [PATCH 1/3] [openacc][openmp] Add dialect representation for acc
 atomic operations

The OpenACC standard specifies an `atomic` construct in section 2.12
(of 3.3 spec), used to ensure that a specific location is accessed or
updated atomically. Four different clauses are allowed: `read`, `write`,
`update`, or `capture`. If no clause appears, it is as if `update`
is used.

The OpenMP specification defines the same clauses for `omp atomic`. The
types of expression and the clauses in the OpenACC spec match the OpenMP
spec exactly. The main difference is that the OpenMP specification is a
superset - it includes clauses for `hint` and `memory order`. It also
allows conditional expression statements. But otherwise, the expression
definition matches.

Thus, for OpenACC, we refactor and reuse the OpenMP implementation as
follows:
* The atomic operations are duplicated in OpenACC dialect. This is
preferable so that each language's semantics are precisely represented
even if specs have divergence.
* However, since semantics overlap, a common interface between the
atomic operations is being added. The semantics for the interfaces
are not generic enough to be used outside of OpenACC and OpenMP, and
thus new folders were added to hold common pieces of the two dialects.
* The atomic interfaces define common accessors (such as getting `x`
or `v`) which match the OpenMP and OpenACC specs. It also adds common
verifiers intended to be called by each dialect's operation verifier.
* The OpenMP write operation was updated to use `x` and `expr` to be
consistent with its other operations (that use naming based on spec).

The frontend lowering necessary to generate the dialect can also be
reused. This will be done in a follow up change.

Differential Revision: https://reviews.llvm.org/D158768
---
 mlir/include/mlir/Dialect/CMakeLists.txt      |   1 +
 mlir/include/mlir/Dialect/OpenACC/OpenACC.h   |   2 +
 .../mlir/Dialect/OpenACC/OpenACCOps.td        | 155 ++++++++-
 .../Dialect/OpenACCMPCommon/CMakeLists.txt    |   1 +
 .../Interfaces/AtomicInterfaces.h             |  22 ++
 .../Interfaces/AtomicInterfaces.td            | 321 ++++++++++++++++++
 .../OpenACCMPCommon/Interfaces/CMakeLists.txt |   1 +
 .../mlir/Dialect/OpenMP/OpenMPDialect.h       |   1 +
 mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td |  52 ++-
 mlir/lib/Dialect/CMakeLists.txt               |   1 +
 mlir/lib/Dialect/OpenACC/CMakeLists.txt       |   1 +
 mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp       |  85 ++++-
 .../Dialect/OpenACCMPCommon/CMakeLists.txt    |   1 +
 .../Interfaces/AtomicInterfaces.cpp           |  11 +
 .../OpenACCMPCommon/Interfaces/CMakeLists.txt |  12 +
 mlir/lib/Dialect/OpenMP/CMakeLists.txt        |   1 +
 mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp  |  95 +-----
 .../OpenMP/OpenMPToLLVMIRTranslation.cpp      |  10 +-
 mlir/test/Dialect/OpenACC/ops.mlir            | 146 ++++++++
 mlir/test/Dialect/OpenMP/invalid.mlir         |   8 +-
 20 files changed, 798 insertions(+), 129 deletions(-)
 create mode 100644 mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
 create mode 100644 mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h
 create mode 100644 mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
 create mode 100644 mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
 create mode 100644 mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
 create mode 100644 mlir/lib/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.cpp
 create mode 100644 mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt

diff --git a/mlir/include/mlir/Dialect/CMakeLists.txt b/mlir/include/mlir/Dialect/CMakeLists.txt
index f0b22654934599..9082d9633339c9 100644
--- a/mlir/include/mlir/Dialect/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/CMakeLists.txt
@@ -22,6 +22,7 @@ add_subdirectory(MemRef)
 add_subdirectory(MLProgram)
 add_subdirectory(NVGPU)
 add_subdirectory(OpenACC)
+add_subdirectory(OpenACCMPCommon)
 add_subdirectory(OpenMP)
 add_subdirectory(PDL)
 add_subdirectory(PDLInterp)
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACC.h b/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
index f294df85ce96ea..d75cc1dedc098a 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
@@ -16,12 +16,14 @@
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/OpDefinition.h"
+#include "mlir/IR/PatternMatch.h"
 #include "mlir/IR/SymbolTable.h"
 
 #include "mlir/Bytecode/BytecodeOpInterface.h"
 #include "mlir/Dialect/OpenACC/OpenACCOpsDialect.h.inc"
 #include "mlir/Dialect/OpenACC/OpenACCOpsEnums.h.inc"
 #include "mlir/Dialect/OpenACC/OpenACCTypeInterfaces.h.inc"
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h"
 #include "mlir/Interfaces/ControlFlowInterfaces.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 58c763995ebe59..e6525e3f878669 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -22,6 +22,7 @@ include "mlir/IR/SymbolInterfaces.td"
 include "mlir/Dialect/OpenACC/OpenACCBase.td"
 include "mlir/Dialect/OpenACC/OpenACCOpsTypes.td"
 include "mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td"
+include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td"
 
 // AccCommon requires definition of OpenACC_Dialect.
 include "mlir/Dialect/OpenACC/AccCommon.td"
@@ -1191,13 +1192,14 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
 
 // Yield operation for the acc.loop and acc.parallel operations.
 def OpenACC_YieldOp : OpenACC_Op<"yield", [ReturnLike, Terminator,
-    ParentOneOf<["FirstprivateRecipeOp, LoopOp, ParallelOp, PrivateRecipeOp, ReductionRecipeOp, SerialOp"]>]> {
+    ParentOneOf<["FirstprivateRecipeOp, LoopOp, ParallelOp, PrivateRecipeOp,"
+                 "ReductionRecipeOp, SerialOp, AtomicUpdateOp"]>]> {
   let summary = "Acc yield and termination operation";
 
   let description = [{
     `acc.yield` is a special terminator operation for block inside regions in
-    acc ops (parallel and loop). It returns values to the immediately enclosing
-    acc op.
+    various acc ops (including parallel, loop, atomic.update). It returns values
+    to the immediately enclosing acc op.
   }];
 
   let arguments = (ins Variadic<AnyType>:$operands);
@@ -1207,6 +1209,153 @@ def OpenACC_YieldOp : OpenACC_Op<"yield", [ReturnLike, Terminator,
   let assemblyFormat = "attr-dict ($operands^ `:` type($operands))?";
 }
 
+//===----------------------------------------------------------------------===//
+// 2.12 atomic construct
+//===----------------------------------------------------------------------===//
+
+def AtomicReadOp : OpenACC_Op<"atomic.read", [AllTypesMatch<["x", "v"]>,
+                                              AtomicReadOpInterface]> {
+
+  let summary = "performs an atomic read";
+
+  let description = [{
+    This operation performs an atomic read.
+
+    The operand `x` is the address from where the value is atomically read.
+    The operand `v` is the address where the value is stored after reading.
+  }];
+
+  let arguments = (ins OpenACC_PointerLikeType:$x,
+                       OpenACC_PointerLikeType:$v,
+                       TypeAttr:$element_type);
+  let assemblyFormat = [{
+    $v `=` $x
+    `:` type($x) `,` $element_type attr-dict
+  }];
+  let hasVerifier = 1;
+}
+
+def AtomicWriteOp : OpenACC_Op<"atomic.write",[AtomicWriteOpInterface]> {
+
+  let summary = "performs an atomic write";
+
+  let description = [{
+    This operation performs an atomic write.
+
+    The operand `x` is the address to where the `expr` is atomically
+    written w.r.t. multiple threads. The evaluation of `expr` need not be
+    atomic w.r.t. the write to address. In general, the type(x) must
+    dereference to type(expr).
+  }];
+
+  let arguments = (ins OpenACC_PointerLikeType:$x,
+                       AnyType:$expr);
+  let assemblyFormat = [{
+    $x `=` $expr
+    `:` type($x) `,` type($expr)
+    attr-dict
+  }];
+  let hasVerifier = 1;
+}
+
+def AtomicUpdateOp : OpenACC_Op<"atomic.update",
+                               [SingleBlockImplicitTerminator<"YieldOp">,
+                                RecursiveMemoryEffects,
+                                AtomicUpdateOpInterface]> {
+
+  let summary = "performs an atomic update";
+
+  let description = [{
+    This operation performs an atomic update.
+
+    The operand `x` is exactly the same as the operand `x` in the OpenACC
+    Standard (OpenACC 3.3, section 2.12). It is the address of the variable
+    that is being updated. `x` is atomically read/written.
+
+    The region describes how to update the value of `x`. It takes the value at
+    `x` as an input and must yield the updated value. Only the update to `x` is
+    atomic. Generally the region must have only one instruction, but can
+    potentially have more than one instructions too. The update is sematically
+    similar to a compare-exchange loop based atomic update.
+
+    The syntax of atomic update operation is different from atomic read and
+    atomic write operations. This is because only the host dialect knows how to
+    appropriately update a value. For example, while generating LLVM IR, if
+    there are no special `atomicrmw` instructions for the operation-type
+    combination in atomic update, a compare-exchange loop is generated, where
+    the core update operation is directly translated like regular operations by
+    the host dialect. The front-end must handle semantic checks for allowed
+    operations.
+  }];
+
+  let arguments = (ins Arg<OpenACC_PointerLikeType,
+                           "Address of variable to be updated",
+                           [MemRead, MemWrite]>:$x);
+  let regions = (region SizedRegion<1>:$region);
+  let assemblyFormat = [{
+    $x `:` type($x) $region attr-dict
+  }];
+  let hasVerifier = 1;
+  let hasRegionVerifier = 1;
+  let hasCanonicalizeMethod = 1;
+  let extraClassDeclaration = [{
+    Operation* getFirstOp() {
+      return &getRegion().front().getOperations().front();
+    }
+  }];
+}
+
+def AtomicCaptureOp : OpenACC_Op<"atomic.capture",
+    [SingleBlockImplicitTerminator<"TerminatorOp">,
+     AtomicCaptureOpInterface]> {
+  let summary = "performs an atomic capture";
+  let description = [{
+    This operation performs an atomic capture.
+
+    The region has the following allowed forms:
+
+    ```
+      acc.atomic.capture {
+        acc.atomic.update ...
+        acc.atomic.read ...
+        acc.terminator
+      }
+
+      acc.atomic.capture {
+        acc.atomic.read ...
+        acc.atomic.update ...
+        acc.terminator
+      }
+
+      acc.atomic.capture {
+        acc.atomic.read ...
+        acc.atomic.write ...
+        acc.terminator
+      }
+    ```
+
+  }];
+
+  let regions = (region SizedRegion<1>:$region);
+  let assemblyFormat = [{
+    $region attr-dict
+  }];
+  let hasRegionVerifier = 1;
+  let extraClassDeclaration = [{
+    /// Returns the `atomic.read` operation inside the region, if any.
+    /// Otherwise, it returns nullptr.
+    AtomicReadOp getAtomicReadOp();
+
+    /// Returns the `atomic.write` operation inside the region, if any.
+    /// Otherwise, it returns nullptr.
+    AtomicWriteOp getAtomicWriteOp();
+
+    /// Returns the `atomic.update` operation inside the region, if any.
+    /// Otherwise, it returns nullptr.
+    AtomicUpdateOp getAtomicUpdateOp();
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // 2.13 Declare Directive
 //===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
new file mode 100644
index 00000000000000..ce3d2d412e95aa
--- /dev/null
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(Interfaces)
\ No newline at end of file
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h
new file mode 100644
index 00000000000000..cfe0ec5185bc8f
--- /dev/null
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h
@@ -0,0 +1,22 @@
+//===- DirectiveAtomicInterfaces.h - directive atomic ops interfaces ------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the operation interface for atomic operations used
+// in OpenACC and OpenMP.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES_H_
+#define OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES_H_
+
+#include "mlir/IR/OpDefinition.h"
+#include "mlir/Interfaces/ControlFlowInterfaces.h"
+
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h.inc"
+
+#endif // OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES_H_
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
new file mode 100644
index 00000000000000..cc4e9df1ae1a19
--- /dev/null
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
@@ -0,0 +1,321 @@
+//===- DirectiveAtomicInterfaces.td - atomic interfaces ----*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Defines the operation interface for atomic operations used in OpenACC and 
+// OpenMP.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES
+#define OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES
+
+include "mlir/IR/OpBase.td"
+include "mlir/Interfaces/ControlFlowInterfaces.td"
+
+def AtomicReadOpInterface : OpInterface<"AtomicReadOpInterface"> {
+  let description = [{
+    This interface is used for OpenACC/OpenMP dialect operation that performs an
+    atomic read.
+
+    The interface terminology uses `x` and `v` like the directive
+    specifications:
+      `v = x;`
+    `x` is the address from where the value is atomically read.
+    `v` is the address where the value is stored after reading.
+  }];
+  let cppNamespace = "::mlir::accomp";
+
+  let methods = [
+    InterfaceMethod<[{
+        Common verifier for operation that implements atomic read interface.
+      }],
+      /*retTy=*/"::mlir::LogicalResult",
+      /*methodName=*/"verifyCommon",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        if ($_op.getX() == $_op.getV()) {
+          return $_op.emitError(
+            "read and write must not be to the same location for atomic reads");
+        }
+        return mlir::success();
+      }]
+    >,
+    InterfaceMethod<[{
+        Obtains `x` which is the address from where the value is atomically
+        read.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getX",
+      /*args=*/(ins)
+    >,
+    InterfaceMethod<[{
+        Obtains `v` which is the address where the value is stored after
+        reading.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getV",
+      /*args=*/(ins)
+    >,
+  ];
+}
+
+def AtomicWriteOpInterface : OpInterface<"AtomicWriteOpInterface"> {
+  let description = [{
+    This interface is used for OpenACC/OpenMP dialect operation that performs an
+    atomic write.
+
+    The interface terminology uses `x` and `expr` like the directive
+    specifications:
+      `x = expr;`
+    `x` is the address to where the `expr` is atomically written.
+  }];
+  let cppNamespace = "::mlir::accomp";
+
+  let methods = [
+    InterfaceMethod<[{
+        Common verifier for operation that implements atomic write interface.
+      }],
+      /*retTy=*/"::mlir::LogicalResult",
+      /*methodName=*/"verifyCommon",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        mlir::Type elementType = $_op.getX().getType().getElementType();
+        if (elementType && elementType != $_op.getExpr().getType())
+          return $_op.emitError("address must dereference to value type");
+        return mlir::success();
+      }]
+    >,
+    InterfaceMethod<[{
+        Obtains `x` which is the address to which the value is atomically
+        written to.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getX",
+      /*args=*/(ins)
+    >,
+    InterfaceMethod<[{
+        Obtains `expr` which corresponds to the expression whose value is
+        written to `x`.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getExpr",
+      /*args=*/(ins)
+    >,
+  ];
+}
+
+def AtomicUpdateOpInterface : OpInterface<"AtomicUpdateOpInterface"> {
+  let description = [{
+    This interface is used for OpenACC/OpenMP dialect operation that performs an
+    atomic update.
+
+    The interface terminology uses `x` to specify the address where a value
+    is atomically written/read.
+    
+    Since atomic update expression comes in many forms, this interface requires
+    that the operation uses a region with a single argument to capture the
+    expression.
+
+    The region describes how to update the value of `x`. It takes the value at
+    `x` as an input and must yield the updated value. Only the update to `x` is
+    atomic. Generally the region must have only one instruction, but can
+    potentially have more than one instructions too. The update is sematically
+    similar to a compare-exchange loop based atomic update.
+  }];
+  let cppNamespace = "::mlir::accomp";
+
+  let methods = [
+    InterfaceMethod<[{
+        Obtains `x` which is the address to which the value is atomically
+        written to / read from.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getX",
+      /*args=*/(ins)
+    >,
+    InterfaceMethod<[{
+        Returns true if the new value is same as old value and the operation is
+        a no-op, false otherwise.
+      }],
+      /*retTy=*/"::mlir::Operation *",
+      /*methodName=*/"getFirstOp",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        return &($_op.getRegion().front().getOperations().front());
+      }]
+    >,
+    InterfaceMethod<[{
+        Returns true if the new value is same as old value and the operation is
+        a no-op, false otherwise.
+      }],
+      /*retTy=*/"bool",
+      /*methodName=*/"isNoOp",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        // The atomic update is a no-op if the terminator is the first and only
+        // operation in its region.
+        mlir::Operation* terminator = 
+          llvm::dyn_cast<mlir::RegionBranchTerminatorOpInterface>($_op.getFirstOp());
+        return terminator && terminator->getOperands().front() == 
+          $_op.getRegion().front().getArgument(0);
+      }]
+    >,
+    InterfaceMethod<[{
+        Returns the new value if the operation is equivalent to just a write
+        operation. Otherwise, returns nullptr.
+      }],
+      /*retTy=*/"::mlir::Value",
+      /*methodName=*/"getWriteOpVal",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        mlir::Operation* terminator = 
+          llvm::dyn_cast<mlir::RegionBranchTerminatorOpInterface>($_op.getFirstOp());
+        if (terminator && terminator->getOperands().front() != 
+          $_op.getRegion().front().getArgument(0)) {
+          return terminator->getOperands().front();
+        }
+        return nullptr;
+      }]
+    >,
+    InterfaceMethod<[{
+        Common verifier for operation that implements atomic update interface.
+      }],
+      /*retTy=*/"::mlir::LogicalResult",
+      /*methodName=*/"verifyCommon",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        if ($_op.getRegion().getNumArguments() != 1)
+          return $_op.emitError("the region must accept exactly one argument");
+
+        Type elementType = $_op.getX().getType().getElementType();
+        if (elementType && elementType != $_op.getRegion().getArgument(0).getType()) {
+          return $_op.emitError("the type of the operand must be a pointer type whose "
+                          "element type is the same as that of the region argument");
+        }
+
+        return mlir::success();
+      }]
+    >,
+    InterfaceMethod<[{
+        Common verifier of the required region for operation that implements
+        atomic update interface.
+      }],
+      /*retTy=*/"::mlir::LogicalResult",
+      /*methodName=*/"verifyRegionsCommon",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        mlir::Operation *terminator = $_op.getRegion().front().getTerminator();
+
+        if (terminator->getOperands().size() != 1)
+          return $_op.emitError("only updated value must be returned");
+    
+        if (terminator->getOperands().front().getType() !=
+            $_op.getRegion().getArgument(0).getType())
+          return $_op.emitError("input and yielded value must have the same type");
+
+        return mlir::success();
+      }]
+    >,
+  ];
+}
+
+def AtomicCaptureOpInterface : OpInterface<"AtomicCaptureOpInterface"> {
+  let description = [{
+    This interface is used for OpenACC/OpenMP dialect operation that performs an
+    atomic capture.
+
+    This interface requires a single region with two operations that each
+    implement one of the atomic interfaces. It can be found in one of these
+    forms:
+      `{ atomic.update, atomic.read }`
+      `{ atomic.read, atomic.update }`
+      `{ atomic.read, atomic.write }`
+  }];
+  let cppNamespace = "::mlir::accomp";
+
+  let methods = [
+    InterfaceMethod<[{
+        Returns the first operation in atomic capture region.
+      }],
+      /*retTy=*/"::mlir::Operation *",
+      /*methodName=*/"getFirstOp",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        return &($_op.getRegion().front().getOperations().front());
+      }]
+    >,
+    InterfaceMethod<[{
+        Returns the second operation in atomic capture region.
+      }],
+      /*retTy=*/"::mlir::Operation *",
+      /*methodName=*/"getSecondOp",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        auto &ops = $_op.getRegion().front().getOperations();
+        return ops.getNextNode(ops.front());
+      }]
+    >,
+      InterfaceMethod<[{
+        Common verifier of the required region for operation that implements
+        atomic capture interface.
+      }],
+      /*retTy=*/"::mlir::LogicalResult",
+      /*methodName=*/"verifyRegionsCommon",
+      /*args=*/(ins),
+      /*methodBody=*/"",
+      /*defaultImplementation=*/[{
+        Block::OpListType &ops = $_op.getRegion().front().getOperations();
+        if (ops.size() != 3)
+          return $_op.emitError()
+                << "expected three operations in atomic.capture region (one "
+                    "terminator, and two atomic ops)";
+        auto &firstOp = ops.front();
+        auto &secondOp = *ops.getNextNode(firstOp);
+        auto firstReadStmt = dyn_cast<AtomicReadOpInterface>(firstOp);
+        auto firstUpdateStmt = dyn_cast<AtomicUpdateOpInterface>(firstOp);
+        auto secondReadStmt = dyn_cast<AtomicReadOpInterface>(secondOp);
+        auto secondUpdateStmt = dyn_cast<AtomicUpdateOpInterface>(secondOp);
+        auto secondWriteStmt = dyn_cast<AtomicWriteOpInterface>(secondOp);
+
+        if (!((firstUpdateStmt && secondReadStmt) ||
+              (firstReadStmt && secondUpdateStmt) ||
+              (firstReadStmt && secondWriteStmt)))
+          return ops.front().emitError()
+                << "invalid sequence of operations in the capture region";
+        if (firstUpdateStmt && secondReadStmt &&
+            firstUpdateStmt.getX() != secondReadStmt.getX())
+          return firstUpdateStmt.emitError()
+                << "updated variable in atomic.update must be captured in "
+                    "second operation";
+        if (firstReadStmt && secondUpdateStmt &&
+            firstReadStmt.getX() != secondUpdateStmt.getX())
+          return firstReadStmt.emitError()
+                << "captured variable in atomic.read must be updated in second "
+                    "operation";
+        if (firstReadStmt && secondWriteStmt &&
+            firstReadStmt.getX() != secondWriteStmt.getX())
+          return firstReadStmt.emitError()
+                << "captured variable in atomic.read must be updated in "
+                    "second operation";
+
+        return mlir::success();
+      }]
+    >,
+  ];
+}
+
+#endif // OPENACC_MP_COMMON_INTERFACES_ATOMICINTERFACES
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
new file mode 100644
index 00000000000000..1f0d0ceb99df2a
--- /dev/null
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
@@ -0,0 +1 @@
+add_mlir_interface(AtomicInterfaces)
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h b/mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
index 2f772a207d252a..584ddc170c2f40 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
@@ -14,6 +14,7 @@
 #define MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_
 
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/OpDefinition.h"
 #include "mlir/IR/PatternMatch.h"
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index b1e1fe00b8594a..2870cee2276a04 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -20,6 +20,7 @@ include "mlir/Interfaces/SideEffectInterfaces.td"
 include "mlir/Interfaces/ControlFlowInterfaces.td"
 include "mlir/IR/SymbolInterfaces.td"
 include "mlir/Dialect/LLVMIR/LLVMOpBase.td"
+include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td"
 include "mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td"
 include "mlir/Dialect/OpenMP/OpenMPTypeInterfaces.td"
 
@@ -1366,7 +1367,8 @@ def TaskwaitOp : OpenMP_Op<"taskwait"> {
 // value of the clause) here decomposes handling of this construct into a
 // two-step process.
 
-def AtomicReadOp : OpenMP_Op<"atomic.read", [AllTypesMatch<["x", "v"]>]> {
+def AtomicReadOp : OpenMP_Op<"atomic.read", [AllTypesMatch<["x", "v"]>,
+                                            AtomicReadOpInterface]> {
 
   let summary = "performs an atomic read";
 
@@ -1412,17 +1414,17 @@ def AtomicReadOp : OpenMP_Op<"atomic.read", [AllTypesMatch<["x", "v"]>]> {
   }];
 }
 
-def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
+def AtomicWriteOp : OpenMP_Op<"atomic.write", [AtomicWriteOpInterface]> {
 
   let summary = "performs an atomic write";
 
   let description = [{
     This operation performs an atomic write.
 
-    The operand `address` is the address to where the `value` is atomically
-    written w.r.t. multiple threads. The evaluation of `value` need not be
-    atomic w.r.t. the write to address. In general, the type(address) must
-    dereference to type(value).
+    The operand `x` is the address to where the `expr` is atomically
+    written w.r.t. multiple threads. The evaluation of `expr` need not be
+    atomic w.r.t. the write to address. In general, the type(x) must
+    dereference to type(expr).
 
     `hint` is the value of hint (as specified in the hint clause). It is a
     compile time constant. As the name suggests, this is just a hint for
@@ -1432,37 +1434,38 @@ def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
     can be one of `seq_cst`, `release` or `relaxed`.
   }];
 
-  let arguments = (ins OpenMP_PointerLikeType:$address,
-                       AnyType:$value,
+  let arguments = (ins OpenMP_PointerLikeType:$x,
+                       AnyType:$expr,
                        DefaultValuedOptionalAttr<I64Attr, "0">:$hint_val,
                        OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
   let assemblyFormat = [{
-    $address `=` $value
+    $x `=` $expr
     oilist( `hint` `(` custom<SynchronizationHint>($hint_val) `)`
           | `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`)
-    `:` type($address) `,` type($value)
+    `:` type($x) `,` type($expr)
     attr-dict
   }];
   let hasVerifier = 1;
   let extraClassDeclaration = [{
     /// The number of variable operands.
     unsigned getNumVariableOperands() {
-      assert(getAddress() && "expected address operand");
-      assert(getValue() && "expected value operand");
+      assert(getX() && "expected address operand");
+      assert(getExpr() && "expected value operand");
       return 2;
     }
 
     /// The i-th variable operand passed.
     Value getVariableOperand(unsigned i) {
       assert(i < 2 && "invalid index position for an operand");
-      return i == 0 ? getAddress() : getValue();
+      return i == 0 ? getX() : getExpr();
     }
   }];
 }
 
 def AtomicUpdateOp : OpenMP_Op<"atomic.update",
                                [SingleBlockImplicitTerminator<"YieldOp">,
-                                RecursiveMemoryEffects]> {
+                                RecursiveMemoryEffects,
+                                AtomicUpdateOpInterface]> {
 
   let summary = "performs an atomic update";
 
@@ -1510,18 +1513,6 @@ def AtomicUpdateOp : OpenMP_Op<"atomic.update",
   let hasRegionVerifier = 1;
   let hasCanonicalizeMethod = 1;
   let extraClassDeclaration = [{
-    Operation* getFirstOp() {
-      return &getRegion().front().getOperations().front();
-    }
-
-    /// Returns true if the new value is same as old value and the operation is
-    /// a no-op, false otherwise.
-    bool isNoOp();
-
-    /// Returns the new value if the operation is equivalent to just a write
-    /// operation. Otherwise, returns nullptr.
-    Value getWriteOpVal();
-
     /// The number of variable operands.
     unsigned getNumVariableOperands() {
       assert(getX() && "expected 'x' operand");
@@ -1537,7 +1528,8 @@ def AtomicUpdateOp : OpenMP_Op<"atomic.update",
 }
 
 def AtomicCaptureOp : OpenMP_Op<"atomic.capture",
-    [SingleBlockImplicitTerminator<"TerminatorOp">]> {
+    [SingleBlockImplicitTerminator<"TerminatorOp">,
+     AtomicCaptureOpInterface]> {
   let summary = "performs an atomic capture";
   let description = [{
     This operation performs an atomic capture.
@@ -1583,12 +1575,6 @@ def AtomicCaptureOp : OpenMP_Op<"atomic.capture",
   let hasRegionVerifier = 1;
   let hasVerifier = 1;
   let extraClassDeclaration = [{
-    /// Returns the first operation in atomic capture region
-    Operation* getFirstOp();
-
-    /// Returns the second operation in atomic capture region
-    Operation* getSecondOp();
-
     /// Returns the `atomic.read` operation inside the region, if any.
     /// Otherwise, it returns nullptr.
     AtomicReadOp getAtomicReadOp();
diff --git a/mlir/lib/Dialect/CMakeLists.txt b/mlir/lib/Dialect/CMakeLists.txt
index 0d7525fa4069f4..d9f6b0fb7e63c2 100644
--- a/mlir/lib/Dialect/CMakeLists.txt
+++ b/mlir/lib/Dialect/CMakeLists.txt
@@ -22,6 +22,7 @@ add_subdirectory(MemRef)
 add_subdirectory(MLProgram)
 add_subdirectory(NVGPU)
 add_subdirectory(OpenACC)
+add_subdirectory(OpenACCMPCommon)
 add_subdirectory(OpenMP)
 add_subdirectory(PDL)
 add_subdirectory(PDLInterp)
diff --git a/mlir/lib/Dialect/OpenACC/CMakeLists.txt b/mlir/lib/Dialect/OpenACC/CMakeLists.txt
index 0f8f165d76b2f3..49a1e216b9381f 100644
--- a/mlir/lib/Dialect/OpenACC/CMakeLists.txt
+++ b/mlir/lib/Dialect/OpenACC/CMakeLists.txt
@@ -14,5 +14,6 @@ add_mlir_dialect_library(MLIROpenACCDialect
   MLIRIR
   MLIRLLVMDialect
   MLIRMemRefDialect
+  MLIROpenACCMPCommon
   )
 
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index 8cda5c8c214388..b2480a5186a2e4 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -25,6 +25,17 @@ using namespace acc;
 #include "mlir/Dialect/OpenACC/OpenACCOpsEnums.cpp.inc"
 #include "mlir/Dialect/OpenACC/OpenACCTypeInterfaces.cpp.inc"
 
+namespace {
+/// Model for pointer-like types that already provide a `getElementType` method.
+template <typename T>
+struct PointerLikeModel
+    : public PointerLikeType::ExternalModel<PointerLikeModel<T>, T> {
+  Type getElementType(Type pointer) const {
+    return llvm::cast<T>(pointer).getElementType();
+  }
+};
+} // namespace
+
 //===----------------------------------------------------------------------===//
 // OpenACC operations
 //===----------------------------------------------------------------------===//
@@ -46,8 +57,9 @@ void OpenACCDialect::initialize() {
   // By attaching interfaces here, we make the OpenACC dialect dependent on
   // the other dialects. This is probably better than having dialects like LLVM
   // and memref be dependent on OpenACC.
-  LLVM::LLVMPointerType::attachInterface<PointerLikeType>(*getContext());
-  MemRefType::attachInterface<PointerLikeType>(*getContext());
+  LLVM::LLVMPointerType::attachInterface<
+      PointerLikeModel<LLVM::LLVMPointerType>>(*getContext());
+  MemRefType::attachInterface<PointerLikeModel<MemRefType>>(*getContext());
 }
 
 //===----------------------------------------------------------------------===//
@@ -975,6 +987,75 @@ void EnterDataOp::getCanonicalizationPatterns(RewritePatternSet &results,
   results.add<RemoveConstantIfCondition<EnterDataOp>>(context);
 }
 
+//===----------------------------------------------------------------------===//
+// AtomicReadOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult AtomicReadOp::verify() {
+  return verifyCommon();
+}
+
+//===----------------------------------------------------------------------===//
+// AtomicWriteOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult AtomicWriteOp::verify() {
+  return verifyCommon();
+}
+
+//===----------------------------------------------------------------------===//
+// AtomicUpdateOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult AtomicUpdateOp::canonicalize(AtomicUpdateOp op,
+                                           PatternRewriter &rewriter) {
+  if (op.isNoOp()) {
+    rewriter.eraseOp(op);
+    return success();
+  }
+
+  if (Value writeVal = op.getWriteOpVal()) {
+    rewriter.replaceOpWithNewOp<AtomicWriteOp>(op, op.getX(), writeVal);
+    return success();
+  }
+
+  return failure();
+}
+
+LogicalResult AtomicUpdateOp::verify() {
+  return verifyCommon();
+}
+
+LogicalResult AtomicUpdateOp::verifyRegions() {
+  return verifyRegionsCommon();
+}
+
+//===----------------------------------------------------------------------===//
+// AtomicCaptureOp
+//===----------------------------------------------------------------------===//
+
+AtomicReadOp AtomicCaptureOp::getAtomicReadOp() {
+  if (auto op = dyn_cast<AtomicReadOp>(getFirstOp()))
+    return op;
+  return dyn_cast<AtomicReadOp>(getSecondOp());
+}
+
+AtomicWriteOp AtomicCaptureOp::getAtomicWriteOp() {
+  if (auto op = dyn_cast<AtomicWriteOp>(getFirstOp()))
+    return op;
+  return dyn_cast<AtomicWriteOp>(getSecondOp());
+}
+
+AtomicUpdateOp AtomicCaptureOp::getAtomicUpdateOp() {
+  if (auto op = dyn_cast<AtomicUpdateOp>(getFirstOp()))
+    return op;
+  return dyn_cast<AtomicUpdateOp>(getSecondOp());
+}
+
+LogicalResult AtomicCaptureOp::verifyRegions() {
+  return verifyRegionsCommon();
+}
+
 //===----------------------------------------------------------------------===//
 // DeclareEnterOp
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt b/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
new file mode 100644
index 00000000000000..ce3d2d412e95aa
--- /dev/null
+++ b/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(Interfaces)
\ No newline at end of file
diff --git a/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.cpp b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.cpp
new file mode 100644
index 00000000000000..ca6a290b5a6dd7
--- /dev/null
+++ b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.cpp
@@ -0,0 +1,11 @@
+//===- AtomicInterfaces.cpp - OpenACC/MP atomic interfaces ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h"
+
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.cpp.inc"
diff --git a/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
new file mode 100644
index 00000000000000..1458e9f6322de5
--- /dev/null
+++ b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_mlir_library(MLIROpenACCMPCommon
+AtomicInterfaces.cpp
+
+ADDITIONAL_HEADER_DIRS
+${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/OpenACCMPCommon/Interfaces
+
+DEPENDS
+MLIRAtomicInterfacesIncGen
+
+LINK_LIBS PUBLIC
+MLIRIR
+)
\ No newline at end of file
diff --git a/mlir/lib/Dialect/OpenMP/CMakeLists.txt b/mlir/lib/Dialect/OpenMP/CMakeLists.txt
index d3d8cec434b79f..40b4837484a136 100644
--- a/mlir/lib/Dialect/OpenMP/CMakeLists.txt
+++ b/mlir/lib/Dialect/OpenMP/CMakeLists.txt
@@ -13,4 +13,5 @@ add_mlir_dialect_library(MLIROpenMPDialect
   MLIRIR
   MLIRLLVMDialect
   MLIRFuncDialect
+  MLIROpenACCMPCommon
   )
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 2ba5f1aca9cf6b..18da93bc1a342b 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -13,6 +13,7 @@
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/Dialect/LLVMIR/LLVMTypes.h"
+#include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/DialectImplementation.h"
 #include "mlir/IR/OpImplementation.h"
@@ -1266,6 +1267,9 @@ LogicalResult OrderedRegionOp::verify() {
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicReadOp::verify() {
+  if (verifyCommon().failed())
+    return mlir::failure();
+
   if (auto mo = getMemoryOrderVal()) {
     if (*mo == ClauseMemoryOrderKind::Acq_rel ||
         *mo == ClauseMemoryOrderKind::Release) {
@@ -1273,9 +1277,6 @@ LogicalResult AtomicReadOp::verify() {
           "memory-order must not be acq_rel or release for atomic reads");
     }
   }
-  if (getX() == getV())
-    return emitError(
-        "read and write must not be to the same location for atomic reads");
   return verifySynchronizationHint(*this, getHintVal());
 }
 
@@ -1284,6 +1285,9 @@ LogicalResult AtomicReadOp::verify() {
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicWriteOp::verify() {
+  if (verifyCommon().failed())
+    return mlir::failure();
+
   if (auto mo = getMemoryOrderVal()) {
     if (*mo == ClauseMemoryOrderKind::Acq_rel ||
         *mo == ClauseMemoryOrderKind::Acquire) {
@@ -1291,10 +1295,6 @@ LogicalResult AtomicWriteOp::verify() {
           "memory-order must not be acq_rel or acquire for atomic writes");
     }
   }
-  Type elementType =
-      llvm::cast<PointerLikeType>(getAddress().getType()).getElementType();
-  if (elementType && elementType != getValue().getType())
-    return emitError("address must dereference to value type");
   return verifySynchronizationHint(*this, getHintVal());
 }
 
@@ -1302,20 +1302,6 @@ LogicalResult AtomicWriteOp::verify() {
 // Verifier for AtomicUpdateOp
 //===----------------------------------------------------------------------===//
 
-bool AtomicUpdateOp::isNoOp() {
-  YieldOp yieldOp = dyn_cast<omp::YieldOp>(getFirstOp());
-  return (yieldOp &&
-          yieldOp.getResults().front() == getRegion().front().getArgument(0));
-}
-
-Value AtomicUpdateOp::getWriteOpVal() {
-  YieldOp yieldOp = dyn_cast<omp::YieldOp>(getFirstOp());
-  if (yieldOp &&
-      yieldOp.getResults().front() != getRegion().front().getArgument(0))
-    return yieldOp.getResults().front();
-  return nullptr;
-}
-
 LogicalResult AtomicUpdateOp::canonicalize(AtomicUpdateOp op,
                                            PatternRewriter &rewriter) {
   if (op.isNoOp()) {
@@ -1332,6 +1318,9 @@ LogicalResult AtomicUpdateOp::canonicalize(AtomicUpdateOp op,
 }
 
 LogicalResult AtomicUpdateOp::verify() {
+  if (verifyCommon().failed())
+    return mlir::failure();
+
   if (auto mo = getMemoryOrderVal()) {
     if (*mo == ClauseMemoryOrderKind::Acq_rel ||
         *mo == ClauseMemoryOrderKind::Acquire) {
@@ -1340,44 +1329,17 @@ LogicalResult AtomicUpdateOp::verify() {
     }
   }
 
-  if (getRegion().getNumArguments() != 1)
-    return emitError("the region must accept exactly one argument");
-
-  Type elementType =
-      llvm::cast<PointerLikeType>(getX().getType()).getElementType();
-  if (elementType && elementType != getRegion().getArgument(0).getType()) {
-    return emitError("the type of the operand must be a pointer type whose "
-                     "element type is the same as that of the region argument");
-  }
-
   return verifySynchronizationHint(*this, getHintVal());
 }
 
 LogicalResult AtomicUpdateOp::verifyRegions() {
-
-  YieldOp yieldOp = *getRegion().getOps<YieldOp>().begin();
-
-  if (yieldOp.getResults().size() != 1)
-    return emitError("only updated value must be returned");
-  if (yieldOp.getResults().front().getType() !=
-      getRegion().getArgument(0).getType())
-    return emitError("input and yielded value must have the same type");
-  return success();
+  return verifyRegionsCommon();
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for AtomicCaptureOp
 //===----------------------------------------------------------------------===//
 
-Operation *AtomicCaptureOp::getFirstOp() {
-  return &getRegion().front().getOperations().front();
-}
-
-Operation *AtomicCaptureOp::getSecondOp() {
-  auto &ops = getRegion().front().getOperations();
-  return ops.getNextNode(ops.front());
-}
-
 AtomicReadOp AtomicCaptureOp::getAtomicReadOp() {
   if (auto op = dyn_cast<AtomicReadOp>(getFirstOp()))
     return op;
@@ -1401,39 +1363,8 @@ LogicalResult AtomicCaptureOp::verify() {
 }
 
 LogicalResult AtomicCaptureOp::verifyRegions() {
-  Block::OpListType &ops = getRegion().front().getOperations();
-  if (ops.size() != 3)
-    return emitError()
-           << "expected three operations in omp.atomic.capture region (one "
-              "terminator, and two atomic ops)";
-  auto &firstOp = ops.front();
-  auto &secondOp = *ops.getNextNode(firstOp);
-  auto firstReadStmt = dyn_cast<AtomicReadOp>(firstOp);
-  auto firstUpdateStmt = dyn_cast<AtomicUpdateOp>(firstOp);
-  auto secondReadStmt = dyn_cast<AtomicReadOp>(secondOp);
-  auto secondUpdateStmt = dyn_cast<AtomicUpdateOp>(secondOp);
-  auto secondWriteStmt = dyn_cast<AtomicWriteOp>(secondOp);
-
-  if (!((firstUpdateStmt && secondReadStmt) ||
-        (firstReadStmt && secondUpdateStmt) ||
-        (firstReadStmt && secondWriteStmt)))
-    return ops.front().emitError()
-           << "invalid sequence of operations in the capture region";
-  if (firstUpdateStmt && secondReadStmt &&
-      firstUpdateStmt.getX() != secondReadStmt.getX())
-    return firstUpdateStmt.emitError()
-           << "updated variable in omp.atomic.update must be captured in "
-              "second operation";
-  if (firstReadStmt && secondUpdateStmt &&
-      firstReadStmt.getX() != secondUpdateStmt.getX())
-    return firstReadStmt.emitError()
-           << "captured variable in omp.atomic.read must be updated in second "
-              "operation";
-  if (firstReadStmt && secondWriteStmt &&
-      firstReadStmt.getX() != secondWriteStmt.getAddress())
-    return firstReadStmt.emitError()
-           << "captured variable in omp.atomic.read must be updated in "
-              "second operation";
+  if (verifyRegionsCommon().failed())
+    return mlir::failure();
 
   if (getFirstOp()->getAttr("hint_val") || getSecondOp()->getAttr("hint_val"))
     return emitOpError(
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 0304c2c07cbf0e..3181b5710c6070 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1217,9 +1217,9 @@ convertOmpAtomicWrite(Operation &opInst, llvm::IRBuilderBase &builder,
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   llvm::AtomicOrdering ao = convertAtomicOrdering(writeOp.getMemoryOrderVal());
-  llvm::Value *expr = moduleTranslation.lookupValue(writeOp.getValue());
-  llvm::Value *dest = moduleTranslation.lookupValue(writeOp.getAddress());
-  llvm::Type *ty = moduleTranslation.convertType(writeOp.getValue().getType());
+  llvm::Value *expr = moduleTranslation.lookupValue(writeOp.getExpr());
+  llvm::Value *dest = moduleTranslation.lookupValue(writeOp.getX());
+  llvm::Type *ty = moduleTranslation.convertType(writeOp.getExpr().getType());
   llvm::OpenMPIRBuilder::AtomicOpValue x = {dest, ty, /*isSigned=*/false,
                                             /*isVolatile=*/false};
   builder.restoreIP(ompBuilder->createAtomicWrite(ompLoc, x, expr, ao));
@@ -1330,7 +1330,7 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
 
   if (atomicWriteOp) {
     isPostfixUpdate = true;
-    mlirExpr = atomicWriteOp.getValue();
+    mlirExpr = atomicWriteOp.getExpr();
   } else {
     isPostfixUpdate = atomicCaptureOp.getSecondOp() ==
                       atomicCaptureOp.getAtomicUpdateOp().getOperation();
@@ -1380,7 +1380,7 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
   auto updateFn = [&](llvm::Value *atomicx,
                       llvm::IRBuilder<> &builder) -> llvm::Value * {
     if (atomicWriteOp)
-      return moduleTranslation.lookupValue(atomicWriteOp.getValue());
+      return moduleTranslation.lookupValue(atomicWriteOp.getExpr());
     Block &bb = *atomicUpdateOp.getRegion().begin();
     moduleTranslation.mapValue(*atomicUpdateOp.getRegion().args_begin(),
                                atomicx);
diff --git a/mlir/test/Dialect/OpenACC/ops.mlir b/mlir/test/Dialect/OpenACC/ops.mlir
index ab39a4192deb1a..2c296a5087be51 100644
--- a/mlir/test/Dialect/OpenACC/ops.mlir
+++ b/mlir/test/Dialect/OpenACC/ops.mlir
@@ -1726,3 +1726,149 @@ acc.set default_async(%i32Value : i32)
 // CHECK: acc.set device_num([[IDXVALUE]] : index)
 // CHECK: acc.set device_num([[IDXVALUE]] : index) if([[IFCOND]])
 // CHECK: acc.set default_async([[I32VALUE]] : i32)
+
+// -----
+
+// CHECK-LABEL: func.func @acc_atomic_read
+// CHECK-SAME: (%[[v:.*]]: memref<i32>, %[[x:.*]]: memref<i32>)
+func.func @acc_atomic_read(%v: memref<i32>, %x: memref<i32>) {
+  // CHECK: acc.atomic.read %[[v]] = %[[x]] : memref<i32>, i32
+  acc.atomic.read %v = %x : memref<i32>, i32
+  return
+}
+
+// -----
+
+// CHECK-LABEL: func.func @acc_atomic_write
+// CHECK-SAME: (%[[ADDR:.*]]: memref<i32>, %[[VAL:.*]]: i32)
+func.func @acc_atomic_write(%addr : memref<i32>, %val : i32) {
+  // CHECK: acc.atomic.write %[[ADDR]] = %[[VAL]] : memref<i32>, i32
+  acc.atomic.write %addr = %val : memref<i32>, i32
+  return
+}
+
+// -----
+
+// CHECK-LABEL: func.func @acc_atomic_update
+// CHECK-SAME: (%[[X:.*]]: memref<i32>, %[[EXPR:.*]]: i32, %[[XBOOL:.*]]: memref<i1>, %[[EXPRBOOL:.*]]: i1)
+func.func @acc_atomic_update(%x : memref<i32>, %expr : i32, %xBool : memref<i1>, %exprBool : i1) {
+  // CHECK: acc.atomic.update %[[X]] : memref<i32>
+  // CHECK-NEXT: (%[[XVAL:.*]]: i32):
+  // CHECK-NEXT:   %[[NEWVAL:.*]] = llvm.add %[[XVAL]], %[[EXPR]] : i32
+  // CHECK-NEXT:   acc.yield %[[NEWVAL]] : i32
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.yield %newval : i32
+  }
+  // CHECK: acc.atomic.update %[[XBOOL]] : memref<i1>
+  // CHECK-NEXT: (%[[XVAL:.*]]: i1):
+  // CHECK-NEXT:   %[[NEWVAL:.*]] = llvm.and %[[XVAL]], %[[EXPRBOOL]] : i1
+  // CHECK-NEXT:   acc.yield %[[NEWVAL]] : i1
+  acc.atomic.update %xBool : memref<i1> {
+  ^bb0(%xval: i1):
+    %newval = llvm.and %xval, %exprBool : i1
+    acc.yield %newval : i1
+  }
+  // CHECK: acc.atomic.update %[[X]] : memref<i32>
+  // CHECK-NEXT: (%[[XVAL:.*]]: i32):
+  // CHECK-NEXT:   %[[NEWVAL:.*]] = llvm.shl %[[XVAL]], %[[EXPR]] : i32
+  // CHECK-NEXT:   acc.yield %[[NEWVAL]] : i32
+  // CHECK-NEXT: }
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.shl %xval, %expr : i32
+    acc.yield %newval : i32
+  }
+  // CHECK: acc.atomic.update %[[X]] : memref<i32>
+  // CHECK-NEXT: (%[[XVAL:.*]]: i32):
+  // CHECK-NEXT:   %[[NEWVAL:.*]] = llvm.intr.smax(%[[XVAL]], %[[EXPR]]) : (i32, i32) -> i32
+  // CHECK-NEXT:   acc.yield %[[NEWVAL]] : i32
+  // CHECK-NEXT: }
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.intr.smax(%xval, %expr) : (i32, i32) -> i32
+    acc.yield %newval : i32
+  }
+
+  // CHECK: acc.atomic.update %[[XBOOL]] : memref<i1>
+  // CHECK-NEXT: (%[[XVAL:.*]]: i1):
+  // CHECK-NEXT:   %[[NEWVAL:.*]] = llvm.icmp "eq" %[[XVAL]], %[[EXPRBOOL]] : i1
+  // CHECK-NEXT:   acc.yield %[[NEWVAL]] : i1
+  // }
+  acc.atomic.update %xBool : memref<i1> {
+  ^bb0(%xval: i1):
+    %newval = llvm.icmp "eq" %xval, %exprBool : i1
+    acc.yield %newval : i1
+  }
+
+  // CHECK: acc.atomic.update %[[X]] : memref<i32> {
+  // CHECK-NEXT: (%[[XVAL:.*]]: i32):
+  // CHECK-NEXT:   acc.yield %[[XVAL]] : i32
+  // CHECK-NEXT: }
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval:i32):
+    acc.yield %xval : i32
+  }
+
+  // CHECK: acc.atomic.update %[[X]] : memref<i32> {
+  // CHECK-NEXT: (%[[XVAL:.*]]: i32):
+  // CHECK-NEXT:   acc.yield %{{.+}} : i32
+  // CHECK-NEXT: }
+  %const = arith.constant 42 : i32
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval:i32):
+    acc.yield %const : i32
+  }
+
+  return
+}
+
+// -----
+
+// CHECK-LABEL: func.func @acc_atomic_capture
+// CHECK-SAME: (%[[v:.*]]: memref<i32>, %[[x:.*]]: memref<i32>, %[[expr:.*]]: i32)
+func.func @acc_atomic_capture(%v: memref<i32>, %x: memref<i32>, %expr: i32) {
+  // CHECK: acc.atomic.capture {
+  // CHECK-NEXT: acc.atomic.update %[[x]] : memref<i32>
+  // CHECK-NEXT: (%[[xval:.*]]: i32):
+  // CHECK-NEXT:   %[[newval:.*]] = llvm.add %[[xval]], %[[expr]] : i32
+  // CHECK-NEXT:   acc.yield %[[newval]] : i32
+  // CHECK-NEXT: }
+  // CHECK-NEXT: acc.atomic.read %[[v]] = %[[x]] : memref<i32>, i32
+  // CHECK-NEXT: }
+  acc.atomic.capture {
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+    acc.atomic.read %v = %x : memref<i32>, i32
+  }
+  // CHECK: acc.atomic.capture {
+  // CHECK-NEXT: acc.atomic.read %[[v]] = %[[x]] : memref<i32>, i32
+  // CHECK-NEXT: acc.atomic.update %[[x]] : memref<i32>
+  // CHECK-NEXT: (%[[xval:.*]]: i32):
+  // CHECK-NEXT:   %[[newval:.*]] = llvm.add %[[xval]], %[[expr]] : i32
+  // CHECK-NEXT:   acc.yield %[[newval]] : i32
+  // CHECK-NEXT: }
+  // CHECK-NEXT: }
+  acc.atomic.capture {
+    acc.atomic.read %v = %x : memref<i32>, i32
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+  }
+  // CHECK: acc.atomic.capture {
+  // CHECK-NEXT: acc.atomic.read %[[v]] = %[[x]] : memref<i32>, i32
+  // CHECK-NEXT: acc.atomic.write %[[x]] = %[[expr]] : memref<i32>, i32
+  // CHECK-NEXT: }
+  acc.atomic.capture {
+    acc.atomic.read %v = %x : memref<i32>, i32
+    acc.atomic.write %x = %expr : memref<i32>, i32
+  }
+
+  return
+}
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index 5a244ec1bebf23..a3552a781669f2 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -874,7 +874,7 @@ func.func @omp_atomic_update(%x: memref<i32>, %expr: i32) {
 // -----
 
 func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
-  // expected-error @below {{expected three operations in omp.atomic.capture region}}
+  // expected-error @below {{expected three operations in atomic.capture region}}
   omp.atomic.capture {
     omp.atomic.read %v = %x : memref<i32>, i32
     omp.terminator
@@ -974,7 +974,7 @@ func.func @omp_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
 
 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 omp.atomic.update must be captured in second operation}}
+    // 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
@@ -989,7 +989,7 @@ func.func @omp_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>,
 
 func.func @omp_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>, %expr: i32) {
   omp.atomic.capture {
-    // expected-error @below {{captured variable in omp.atomic.read must be updated in second operation}}
+    // expected-error @below {{captured variable in atomic.read must be updated in second operation}}
     omp.atomic.read %v = %y : memref<i32>, i32
     omp.atomic.update %x : memref<i32> {
     ^bb0(%xval: i32):
@@ -1004,7 +1004,7 @@ func.func @omp_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>,
 
 func.func @omp_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>, %expr: i32) {
   omp.atomic.capture {
-    // expected-error @below {{captured variable in omp.atomic.read must be updated in second operation}}
+    // expected-error @below {{captured variable in atomic.read must be updated in second operation}}
     omp.atomic.read %v = %x : memref<i32>, i32
     omp.atomic.write %y = %expr : memref<i32>, i32
     omp.terminator

>From c15861c980ad0e5286c91f70cd2989a2ceb537b6 Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Wed, 6 Sep 2023 09:47:37 -0700
Subject: [PATCH 2/3] recursive memory effects, newlines, fix description

* Add RecursiveMemoryEffects to both acc and omp atomic caputre
* Add newlines to the CMakeLists.txt files
* Fix descriptions in atomic interfaces
---
 mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td              | 2 +-
 mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt     | 2 +-
 .../Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td   | 5 ++---
 mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td                | 2 +-
 mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt              | 2 +-
 mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt   | 2 +-
 6 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index e6525e3f878669..0032ffa94dede9 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -1307,7 +1307,7 @@ def AtomicUpdateOp : OpenACC_Op<"atomic.update",
 
 def AtomicCaptureOp : OpenACC_Op<"atomic.capture",
     [SingleBlockImplicitTerminator<"TerminatorOp">,
-     AtomicCaptureOpInterface]> {
+     RecursiveMemoryEffects, AtomicCaptureOpInterface]> {
   let summary = "performs an atomic capture";
   let description = [{
     This operation performs an atomic capture.
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
index ce3d2d412e95aa..3ac45c187158aa 100644
--- a/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/CMakeLists.txt
@@ -1 +1 @@
-add_subdirectory(Interfaces)
\ No newline at end of file
+add_subdirectory(Interfaces)
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
index cc4e9df1ae1a19..57063ca763d2f2 100644
--- a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
@@ -126,7 +126,7 @@ def AtomicUpdateOpInterface : OpInterface<"AtomicUpdateOpInterface"> {
     The region describes how to update the value of `x`. It takes the value at
     `x` as an input and must yield the updated value. Only the update to `x` is
     atomic. Generally the region must have only one instruction, but can
-    potentially have more than one instructions too. The update is sematically
+    potentially have more than one instructions too. The update is semantically
     similar to a compare-exchange loop based atomic update.
   }];
   let cppNamespace = "::mlir::accomp";
@@ -141,8 +141,7 @@ def AtomicUpdateOpInterface : OpInterface<"AtomicUpdateOpInterface"> {
       /*args=*/(ins)
     >,
     InterfaceMethod<[{
-        Returns true if the new value is same as old value and the operation is
-        a no-op, false otherwise.
+        Returns the first operation in atomic update region.
       }],
       /*retTy=*/"::mlir::Operation *",
       /*methodName=*/"getFirstOp",
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 2870cee2276a04..86fe62e77b535b 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -1529,7 +1529,7 @@ def AtomicUpdateOp : OpenMP_Op<"atomic.update",
 
 def AtomicCaptureOp : OpenMP_Op<"atomic.capture",
     [SingleBlockImplicitTerminator<"TerminatorOp">,
-     AtomicCaptureOpInterface]> {
+     RecursiveMemoryEffects, AtomicCaptureOpInterface]> {
   let summary = "performs an atomic capture";
   let description = [{
     This operation performs an atomic capture.
diff --git a/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt b/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
index ce3d2d412e95aa..3ac45c187158aa 100644
--- a/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
+++ b/mlir/lib/Dialect/OpenACCMPCommon/CMakeLists.txt
@@ -1 +1 @@
-add_subdirectory(Interfaces)
\ No newline at end of file
+add_subdirectory(Interfaces)
diff --git a/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
index 1458e9f6322de5..6da04424231aac 100644
--- a/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
+++ b/mlir/lib/Dialect/OpenACCMPCommon/Interfaces/CMakeLists.txt
@@ -9,4 +9,4 @@ MLIRAtomicInterfacesIncGen
 
 LINK_LIBS PUBLIC
 MLIRIR
-)
\ No newline at end of file
+)

>From 3ed74a1382d838fcb4f575aac184b25c6c8b874e Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Wed, 6 Sep 2023 10:02:40 -0700
Subject: [PATCH 3/3] Add invalid and canonicalize tests for acc atomics

---
 mlir/test/Dialect/OpenACC/canonicalize.mlir |  75 +++++++
 mlir/test/Dialect/OpenACC/invalid.mlir      | 220 ++++++++++++++++++++
 2 files changed, 295 insertions(+)

diff --git a/mlir/test/Dialect/OpenACC/canonicalize.mlir b/mlir/test/Dialect/OpenACC/canonicalize.mlir
index 9b16db98027543..6173ab6699c6c5 100644
--- a/mlir/test/Dialect/OpenACC/canonicalize.mlir
+++ b/mlir/test/Dialect/OpenACC/canonicalize.mlir
@@ -142,3 +142,78 @@ func.func @testhostdataop(%a: memref<f32>, %ifCond: i1) -> () {
 
 // CHECK-LABEL: func.func @testhostdataop
 // CHECK: acc.host_data dataOperands(%{{.*}} : memref<f32>) {
+
+// -----
+
+func.func @update_no_op(%x : memref<i32>) {
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval : i32):
+    acc.yield %xval : i32
+  }
+  return
+}
+
+// CHECK-LABEL: func.func @update_no_op
+// CHECK-NOT: acc.atomic.update
+
+// -----
+
+func.func @update_write_op(%x : memref<i32>, %value: i32) {
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval : i32):
+    acc.yield %value : i32
+  }
+  return
+}
+
+// CHECK-LABEL: func.func @update_write_op
+// CHECK-SAME:            (%[[X:.+]]: memref<i32>, %[[VALUE:.+]]: i32)
+// CHECK: acc.atomic.write %[[X]] = %[[VALUE]] : memref<i32>, i32
+// CHECK-NOT: acc.atomic.update
+
+// -----
+
+func.func @update_normal(%x : memref<i32>, %value: i32) {
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval : i32):
+    %newval = arith.addi %xval, %value : i32
+    acc.yield %newval : i32
+  }
+  return
+}
+
+// CHECK-LABEL: func.func @update_normal
+// CHECK: acc.atomic.update
+// CHECK: arith.addi
+// CHECK: acc.yield
+
+// -----
+
+func.func @update_unnecessary_computations(%x: memref<i32>) {
+  %c0 = arith.constant 0 : i32
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = arith.addi %xval, %c0 : i32
+    acc.yield %newval: i32
+  }
+  return
+}
+
+// CHECK-LABEL: func.func @update_unnecessary_computations
+// CHECK-NOT: acc.atomic.update
+
+// -----
+
+func.func @update_unnecessary_computations(%x: memref<i32>) {
+  %c0 = arith.constant 0 : i32
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = arith.muli %xval, %c0 : i32
+    acc.yield %newval: i32
+  }
+  return
+}
+
+// CHECK-LABEL: func.func @update_unnecessary_computations
+// CHECK-NOT: acc.atomic.update
+// CHECK: acc.atomic.write
diff --git a/mlir/test/Dialect/OpenACC/invalid.mlir b/mlir/test/Dialect/OpenACC/invalid.mlir
index 98ac0b85538cfe..46c1bb51f0e4c0 100644
--- a/mlir/test/Dialect/OpenACC/invalid.mlir
+++ b/mlir/test/Dialect/OpenACC/invalid.mlir
@@ -507,3 +507,223 @@ acc.parallel {
 
 // expected-error at +1 {{'acc.set' op at least one default_async, device_num, or device_type operand must appear}}
 acc.set
+
+// -----
+
+func.func @acc_atomic_write(%addr : memref<memref<i32>>, %val : i32) {
+  // expected-error @below {{address must dereference to value type}}
+  acc.atomic.write %addr = %val : memref<memref<i32>>, i32
+  return
+}
+
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: f32) {
+  // expected-error @below {{the type of the operand must be a pointer type whose element type is the same as that of the region argument}}
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: f32):
+    %newval = llvm.fadd %xval, %expr : f32
+    acc.yield %newval : f32
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
+  // expected-error @+2 {{op expects regions to end with 'acc.yield', found 'acc.terminator'}}
+  // expected-note @below {{in custom textual format, the absence of terminator implies 'acc.yield'}}
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.terminator
+  }
+  return
+}
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
+  // expected-error @below {{invalid kind of type specified}}
+  acc.atomic.update %x : i32 {
+  ^bb0(%xval: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.yield %newval : i32
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
+  // expected-error @below {{only updated value must be returned}}
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.yield %newval, %expr : i32, i32
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: i32, %y: f32) {
+  // expected-error @below {{input and yielded value must have the same type}}
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.yield %y: f32
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
+  // expected-error @below {{the region must accept exactly one argument}}
+  acc.atomic.update %x : memref<i32> {
+  ^bb0(%xval: i32, %tmp: i32):
+    %newval = llvm.add %xval, %expr : i32
+    acc.yield %newval : i32
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_capture(%x: memref<i32>, %v: memref<i32>, %expr: i32) {
+  // expected-error @below {{expected three operations in atomic.capture region}}
+  acc.atomic.capture {
+    acc.atomic.read %v = %x : 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}}
+    acc.atomic.read %v = %x : memref<i32>, i32
+    acc.atomic.read %v = %x : 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}}
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : 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}}
+    acc.atomic.write %x = %expr : memref<i32>, i32
+    acc.atomic.write %x = %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}}
+    acc.atomic.write %x = %expr : memref<i32>, i32
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : 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}}
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+    acc.atomic.write %x = %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}}
+    acc.atomic.write %x = %expr : memref<i32>, i32
+    acc.atomic.read %v = %x : memref<i32>, i32
+    acc.terminator
+  }
+  return
+}
+
+// -----
+
+func.func @acc_atomic_capture(%x: memref<i32>, %y: 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.read %v = %y : memref<i32>, i32
+    acc.terminator
+  }
+}
+
+// -----
+
+func.func @acc_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>, %expr: i32) {
+  acc.atomic.capture {
+    // expected-error @below {{captured variable in atomic.read must be updated in second operation}}
+    acc.atomic.read %v = %y : memref<i32>, i32
+    acc.atomic.update %x : memref<i32> {
+    ^bb0(%xval: i32):
+      %newval = llvm.add %xval, %expr : i32
+      acc.yield %newval : i32
+    }
+    acc.terminator
+  }
+}
+
+// -----
+
+func.func @acc_atomic_capture(%x: memref<i32>, %y: memref<i32>, %v: memref<i32>, %expr: i32) {
+  acc.atomic.capture {
+    // expected-error @below {{captured variable in atomic.read must be updated in second operation}}
+    acc.atomic.read %v = %x : memref<i32>, i32
+    acc.atomic.write %y = %expr : memref<i32>, i32
+    acc.terminator
+  }
+}



More information about the Mlir-commits mailing list