[MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[shraiysh]

Summary

This patch supports the atomic construct (read and write) following
section 2.17.7 of OpenMP 5.0 standard. Also added tests and
verifier for the same.

[abidmalikwaterloo]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

432

Are we taking care of capture ,compare ,fail(seq_cst | acquire | relaxed)
weak?

[shraiysh]

Thanks for reviewing this patch @abidmalikwaterloo.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

432

I am sorry, I did not understand it correctly. The omp.atomic.capture and omp.atomic.update operations will be released as a separate patch to avoid too many changes in one patch (and ease of review). The restriction on memory ordering clause values is verified in the verifiers for these operations independently. Let me know if this wasn't your concern.

[kiranchandramohan]

Thanks @shraiysh for this patch. Have a few comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

63

Is it possible to autogen this from OMP.td? This capability was introduced as part of https://reviews.llvm.org/D84347.

If you can define ClauseVal entries in llvm/include/llvm/Frontend/OpenMP/OMP.td then these entries will be autogenerated. Example for proc_bind given below.

def OMP_PROC_BIND_master : ClauseVal<"master",2,1> {}
def OMP_PROC_BIND_close : ClauseVal<"close",3,1> {}
def OMP_PROC_BIND_spread : ClauseVal<"spread",4,1> {}
def OMP_PROC_BIND_default : ClauseVal<"default",5,0> {}
def OMP_PROC_BIND_unknown : ClauseVal<"unknown",6,0> { let isDefault = 1; }
def OMPC_ProcBind : Clause<"proc_bind"> {

let clangClass = "OMPProcBindClause";
let flangClass = "OmpProcBindClause";
let enumClauseValue = "ProcBindKind";
let allowedClauseValues = [
  OMP_PROC_BIND_master,
  OMP_PROC_BIND_close,
  OMP_PROC_BIND_spread,
  OMP_PROC_BIND_default,
  OMP_PROC_BIND_unknown
];

}

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1163

Consider reducing this to "memory-order must not be acq_rel or release for atomic reads".
Similarly for writes as well.

1201

Is the order of hint and memory_order different from Read?

mlir/test/Dialect/OpenMP/invalid.mlir

349

should we have tests for allowed once clauses?

444

Nit: Add newline

[shraiysh]

Thanks for the review @kiranchandramohan. Addressed comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

63

I don't think it can be done because in OMP.td, these values are treated like independent clauses. There is no unified "memory ordering" clause in that file. Let me know if I am missing something here.

[clementval]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

63

You can still define the enum in the file and have it generated for you by the current back-end.

[shraiysh]

Moved Memory Order clause to OMP.td.

Thanks for the suggestion @kiranchandramohan @clementval. Addressed comments

[shraiysh]

Handle build failure and rebase with main

[shraiysh]

The Synchronization Hint functions have been moved above because they are required in parseClauses. I will add it as an NFC and then rebase here, because right now reviewing this (the way it is showing differences) will be hard.

[shraiysh]

Rebase with main

[shraiysh]

Hi all, the NFC has been merged, and the build is passing. This is ready for review now.

[kiranchandramohan]

The patch looks OK. I just wanted to discuss the syntax also. Would any of the following be better?

%8 = omp.atomic.read %addr : memref<i32> -> i32 hint(speculative, contended) memory_order(seq_cst)

%8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref<i32> -> i32

%8 = omp.atomic.read %addr {hint(speculative, contended) memory_order(seq_cst)} : memref<i32> -> i32

[shraiysh]

Hi @kiranchandramohan. Thanks for the review.

%8 = omp.atomic.read %addr : memref<i32> -> i32 hint(speculative, contended) memory_order(seq_cst)

This cannot be done because the statement ends at optional clauses. This is dangerous in the following situation where parser will try to match return as a clause instead of a new instruction. I had first actually tried it this way. Let me know if there is something that can be done to avoid this problem with this syntax.

%8 = omp.atomic.read %addr : memref<i32> -> i32 memory_order(seq_cst)
return

We can try one of remaining two. I do not have a preference between these three:

%8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref<i32> -> i32

%8 = omp.atomic.read %addr {hint(speculative, contended) memory_order(seq_cst)} : memref<i32> -> i32

%8 = omp.atomic.read hint(speculative, contended) memory_order(seq_cst) %addr : memref<i32> -> i32 // current syntax

[kiranchandramohan]

LGTM.

My preference is the following one. If you agree then please make the change, otherwise, you can keep it as is. Also, wait a couple of days to see whether others have comments.

%8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref<i32> -> i32

[shraiysh]

In D111992#3080308, @kiranchandramohan wrote:

LGTM.

My preference is the following one. If you agree then please make the change, otherwise, you can keep it as is. Also, wait a couple of days to see whether others have comments.

%8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref<i32> -> i32

Sure, will wait for other's comments. Meanwhile, I will update it with this syntax.

[shraiysh]

Rebase with main, updated syntax.

[peixin]

clang/lib/CodeGen/CGStmtOpenMP.cpp

5993 ↗

(On Diff #381772)

The memory_order clause in clang side is not handled in this patch. Maybe leaving the error is better since users will know it is not supported yet in clang.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

430

How do you plan to handle synchronization between the threads if there is no region in atomic read op? Will there be one implicit kmpc_flush function call before !$omp end atomic? If yes, it is easier to find to location of emiting kmpc_flush if we have one region here. Please let me know if I am wrong.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1124

Please also check OpenMP 5.1 Spec. It seems that acq_rel is allowed if read clause is specified.

[peixin]

clang/lib/CodeGen/CGStmtOpenMP.cpp

5993 ↗

(On Diff #381772)

The memory_order clause in clang side is not handled in this patch. Maybe leaving the error is better since users will know it is not supported yet in clang.

Sorry. Please ignore this comment.

[shraiysh]

Thanks for the review @peixin.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

430

Yes - there will be a kmpc_flush call, but OpenMP Dialect does not have to worry about that. OpenMP IR Builder takes care of flushes in the function createAtomicRead.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1124

Thanks for this observation, I will update it with the 5.1 standard.

[shraiysh]

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

1124

Just to be clear, the OpenMP 5.1 specification changes atomic by a considerable amount (especially it breaks capture into two sub-constructs (coming as a separate patch)) and AFAIK we are targeting OpenMP 5.0. So, do I still update it to the 5.1 standard (this will affect further implementation of atomic construct)?

[shraiysh]

Rebase with main

llvm/include/llvm/Frontend/OpenMP/OMP.td

let allowedClauseValues = [
     OMP_SCHEDULE_Dynamic,
     OMP_SCHEDULE_Guided,
     OMP_SCHEDULE_Auto,
     OMP_SCHEDULE_Runtime,
     OMP_SCHEDULE_Default
   ];
 }
 
+def OMP_MEMORY_ORDER_SeqCst : ClauseVal<"seq_cst", 1, 1> {}
+def OMP_MEMORY_ORDER_AcqRel : ClauseVal<"acq_rel", 2, 1> {}
+def OMP_MEMORY_ORDER_Acquire : ClauseVal<"acquire", 3, 1> {}
+def OMP_MEMORY_ORDER_Release : ClauseVal<"release", 4, 1> {}
+def OMP_MEMORY_ORDER_Relaxed : ClauseVal<"relaxed", 5, 1> {}
+def OMP_MEMORY_ORDER_Default : ClauseVal<"default", 6, 0> {
+  let isDefault = 1;
+}
+def OMPC_MemoryOrder : Clause<"memory_order"> {
+  let clangClass = "OMPMemoryOrderClause";
+  let flangClass = "OmpMemoryOrderClause";
+  let enumClauseValue = "MemoryOrderKind";
+  let allowedClauseValues = [
+    OMP_MEMORY_ORDER_SeqCst,
+    OMP_MEMORY_ORDER_AcqRel,
+    OMP_MEMORY_ORDER_Acquire,
+    OMP_MEMORY_ORDER_Release,
+    OMP_MEMORY_ORDER_Relaxed,
+    OMP_MEMORY_ORDER_Default
+  ];
+}
+
 def OMPC_Ordered : Clause<"ordered"> {
   let clangClass = "OMPOrderedClause";
   let flangClass = "ScalarIntConstantExpr";
   let isValueOptional = true;
 }
 def OMPC_NoWait : Clause<"nowait"> {
   let clangClass = "OMPNowaitClause";
 }
 def OMPC_Untied : Clause<"untied"> { let clangClass = "OMPUntiedClause"; }
 def OMPC_Mergeable : Clause<"mergeable"> {
   let clangClass = "OMPMergeableClause";
 }
 def OMPC_Read : Clause<"read"> { let clangClass = "OMPReadClause"; }
 def OMPC_Write : Clause<"write"> { let clangClass = "OMPWriteClause"; }
 def OMPC_Update : Clause<"update"> { let clangClass = "OMPUpdateClause"; }
 def OMPC_Capture : Clause<"capture"> { let clangClass = "OMPCaptureClause"; }
 def OMPC_SeqCst : Clause<"seq_cst"> { let clangClass = "OMPSeqCstClause"; }
 def OMPC_AcqRel : Clause<"acq_rel"> { let clangClass = "OMPAcqRelClause"; }
 def OMPC_Acquire : Clause<"acquire"> { let clangClass = "OMPAcquireClause"; }
 def OMPC_Release : Clause<"release"> { let clangClass = "OMPReleaseClause"; }
 def OMPC_Relaxed : Clause<"relaxed"> { let clangClass = "OMPRelaxedClause"; }
+
 def OMPC_Depend : Clause<"depend"> {
   let clangClass = "OMPDependClause";
   let flangClass = "OmpDependClause";
 }
 def OMPC_Device : Clause<"device"> {
   let clangClass = "OMPDeviceClause";
   let flangClass = "ScalarIntExpr";
 }

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

def OpenMP_PointerLikeType : Type<
   CPred<"$_self.isa<::mlir::omp::PointerLikeType>()">,
   "OpenMP-compatible variable type", "::mlir::omp::PointerLikeType">;
 
 //===----------------------------------------------------------------------===//
 // 2.6 parallel Construct
 //===----------------------------------------------------------------------===//
 
 // Possible values for the default clause
 def ClauseDefaultPrivate : StrEnumAttrCase<"defprivate">;
 def ClauseDefaultFirstPrivate : StrEnumAttrCase<"deffirstprivate">;
 def ClauseDefaultShared : StrEnumAttrCase<"defshared">;
 def ClauseDefaultNone : StrEnumAttrCase<"defnone">;
 
 def ClauseDefault : StrEnumAttr<
     "ClauseDefault",
     "default clause",
     [ClauseDefaultPrivate, ClauseDefaultFirstPrivate, ClauseDefaultShared,
let description = [{
     The taskwait construct specifies a wait on the completion of child tasks
     of the current task.
   }];
 
   let assemblyFormat = "attr-dict";
 }
 
 //===----------------------------------------------------------------------===//
+// 2.17.7 atomic construct
+//===----------------------------------------------------------------------===//
+
+// In the OpenMP Specification, atomic construct has an `atomic-clause` which
+// can take the values `read`, `write`, `update` and `capture`. These four
+// kinds of atomic constructs are fundamentally independent and are handled
+// separately while lowering. Having four separate operations (one for each
+// value of the clause) here decomposes handling of this construct into a
+// two-step process.
+
+def AtomicReadOp : OpenMP_Op<"atomic.read"> {
+  let arguments = (ins OpenMP_PointerLikeType:$address,
+                       DefaultValuedAttr<I64Attr, "0">:$hint,
+                       OptionalAttr<MemoryOrderKind>:$memory_order);
+  let results = (outs AnyType);
+  let parser = [{ return parseAtomicReadOp(parser, result); }];
+  let printer = [{ return printAtomicReadOp(p, *this); }];
+  let verifier = [{ return verifyAtomicReadOp(*this); }];
+}
+
+def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
+  let arguments = (ins OpenMP_PointerLikeType:$address,
+                       AnyType:$value,
+                       DefaultValuedAttr<I64Attr, "0">:$hint,
+                       OptionalAttr<MemoryOrderKind>:$memory_order);
+  let parser = [{ return parseAtomicWriteOp(parser, result); }];
+  let printer = [{ return printAtomicWriteOp(p, *this); }];
+  let verifier = [{ return verifyAtomicWriteOp(*this); }];
+}
+
+//===----------------------------------------------------------------------===//
 // 2.19.5.7 declare reduction Directive
 //===----------------------------------------------------------------------===//
 
 def ReductionDeclareOp : OpenMP_Op<"reduction.declare", [Symbol]> {
   let summary = "declares a reduction kind";
 
   let description = [{
     Declares an OpenMP reduction kind. This requires two mandatory and one

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/Dialect/LLVMIR/LLVMTypes.h"
 #include "mlir/Dialect/StandardOps/IR/Ops.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/OperationSupport.h"
 
+#include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include <cstddef>
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
 #include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"

 // Parser, printer and verifier for Synchronization Hint (2.17.12)
 //===----------------------------------------------------------------------===//
 
 /// Parses a Synchronization Hint clause. The value of hint is an integer
 /// which is a combination of different hints from `omp_sync_hint_t`.
 ///
 /// hint-clause = `hint` `(` hint-value `)`
 static ParseResult parseSynchronizationHint(OpAsmParser &parser,
-                                            IntegerAttr &hintAttr) {
-  if (failed(parser.parseOptionalKeyword("hint"))) {
+                                            IntegerAttr &hintAttr,
+                                            bool parseKeyword = true) {
+  if (parseKeyword && failed(parser.parseOptionalKeyword("hint"))) {
     hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), 0);
     return success();
   }
 
   if (failed(parser.parseLParen()))
     return failure();
   StringRef hintKeyword;
   int64_t hint = 0;
if (contended)
     hints.push_back("contended");
   if (nonspeculative)
     hints.push_back("nonspeculative");
   if (speculative)
     hints.push_back("speculative");
 
   p << "hint(";
   llvm::interleaveComma(hints, p);
-  p << ")";
+  p << ") ";
 }
 
 /// Verifies a synchronization hint clause
-static LogicalResult verifySynchronizationHint(Operation *op, int32_t hint) {
+static LogicalResult verifySynchronizationHint(Operation *op, uint64_t hint) {
 
   // Helper function to get n-th bit from the right end of `value`
   auto bitn = [](int value, int n) -> bool { return value & (1 << n); };
 
   bool uncontended = bitn(hint, 0);
   bool contended = bitn(hint, 1);
   bool nonspeculative = bitn(hint, 2);
   bool speculative = bitn(hint, 3);
if (!decl) {
       return op.emitOpError() << "expected symbol reference " << symbolRef
                               << " to point to a critical declaration";
     }
   }
 
   return success();
 }
 
+//===----------------------------------------------------------------------===//
+// Atomic Construct
+//===----------------------------------------------------------------------===//
+
+/// Parses clauses for atomic construct
+/// atomic-clause-list ::= clause clause-list | empty
+/// clause := hint-clause | memory-order-clause
+static ParseResult parseAtomicClauses(OpAsmParser &parser,
+                                      OperationState &result) {
+  IntegerAttr hint = IntegerAttr::get(parser.getBuilder().getI64Type(), 0);
+  StringRef keyword, memoryOrder;
+  llvm::BitVector done(2, false);
+
+  enum { MemoryOrderClausePos, HintClausePos };
+
+  while (succeeded(parser.parseOptionalKeyword(&keyword))) {
+    if (keyword == "memory_order") {
+      if (done[MemoryOrderClausePos])
+        return allowedOnce(parser, keyword, result.name.getStringRef());
+      if (parser.parseLParen() || parser.parseKeyword(&memoryOrder) ||
+          parser.parseRParen())
+        return failure();
+      result.addAttribute("memory_order",
+                          parser.getBuilder().getStringAttr(memoryOrder));
+      done[MemoryOrderClausePos] = true;
+    } else if (keyword == "hint") {
+      if (done[HintClausePos])
+        return allowedOnce(parser, keyword, result.name.getStringRef());
+      if (parseSynchronizationHint(parser, hint, false))
+        return failure();
+      done[HintClausePos] = true;
+    } else {
+      return parser.emitError(parser.getNameLoc())
+             << keyword << " is not a valid atomic clause";
+    }
+  }
+  result.addAttribute("hint", hint);
+  return success();
+}
+
+//===----------------------------------------------------------------------===//
+// AtomicReadOp
+//===----------------------------------------------------------------------===//
+
+/// Parser for AtomicReadOp
+///
+/// operation ::= `omp.atomic.read` atomic-clause-list address `->` result-type
+/// address ::= operand `:` type
+static ParseResult parseAtomicReadOp(OpAsmParser &parser,
+                                     OperationState &result) {
+  OpAsmParser::OperandType address;
+  Type addressType;
+
+  if (parseAtomicClauses(parser, result) || parser.parseOperand(address) ||
+      parser.parseColonType(addressType) ||
+      parser.resolveOperand(address, addressType, result.operands))
+    return failure();
+
+  SmallVector<Type> resultType;
+  if (parser.parseArrowTypeList(resultType))
+    return failure();
+  result.addTypes(resultType);
+  return success();
+}
+
+/// Printer for AtomicReadOp
+static void printAtomicReadOp(OpAsmPrinter &p, AtomicReadOp op) {
+  p << " ";
+  if (op.memory_order())
+    p << "memory_order(" << op.memory_order().getValue() << ") ";
+  if (op.hintAttr())
+    printSynchronizationHint(p << " ", op, op.hintAttr());
+  p << op.address() << " : " << op.address().getType() << " ";
+  p << " -> " << op.getType();
+  return;
+}
+
+/// Verifier for AtomicReadOp
+static LogicalResult verifyAtomicReadOp(AtomicReadOp op) {
+  if (op.memory_order()) {
+    StringRef memOrder = op.memory_order().getValue();
+    if (memOrder.equals("acq_rel") || memOrder.equals("release"))
+      return op.emitError(
+          "memory-order must not be acq_rel or release for atomic reads");
+  }
+  return verifySynchronizationHint(op, op.hint());
+}
+
+//===----------------------------------------------------------------------===//
+// AtomicWriteOp
+//===----------------------------------------------------------------------===//
+
+/// Parser for AtomicWriteOp
+///
+/// operation ::= `omp.atomic.write` atomic-clause-list operands
+/// operands ::= address `,` value
+/// address ::= operand `:` type
+/// value ::= operand `:` type
+static ParseResult parseAtomicWriteOp(OpAsmParser &parser,
+                                      OperationState &result) {
+  OpAsmParser::OperandType address, value;
+  Type addrType, valueType;
+  if (parseAtomicClauses(parser, result) || parser.parseOperand(address) ||
+      parser.parseColonType(addrType) ||
+      parser.resolveOperand(address, addrType, result.operands) ||
+      parser.parseComma() || parser.parseOperand(value) ||
+      parser.parseColonType(valueType) ||
+      parser.resolveOperand(value, valueType, result.operands))
+    return failure();
+  return success();
+}
+
+/// Printer for AtomicWriteOp
+static void printAtomicWriteOp(OpAsmPrinter &p, AtomicWriteOp op) {
+  p << " ";
+  if (op.memory_order())
+    p << "memory_order(" << op.memory_order() << ") ";
+  if (op.hintAttr())
+    printSynchronizationHint(p, op, op.hintAttr());
+  p << op.address() << " : " << op.address().getType() << ", " << op.value()
+    << " : " << op.value().getType();
+  return;
+}
+
+/// Verifier for AtomicWriteOp
+static LogicalResult verifyAtomicWriteOp(AtomicWriteOp op) {
+  if (op.memory_order()) {
+    StringRef memoryOrder = op.memory_order().getValue();
+    if (memoryOrder.equals("acq_rel") || memoryOrder.equals("acquire"))
+      return op.emitError(
+          "memory-order must not be acq_rel or acquire for atomic writes");
+  }
+  return verifySynchronizationHint(op, op.hint());
+}
+
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"

mlir/test/Dialect/OpenMP/invalid.mlir

 
 omp.critical.declare @mutex
 func @omp_critica() -> () {
   // expected-error @below {{invalid_hint is not a valid hint}}
   omp.critical(@mutex) hint(invalid_hint) {
     omp.terminator
   }
 }
+
+// -----
+
+func @omp_atomic_read1(%addr : memref<i32>) {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
+  %1 = omp.atomic.read hint(speculative, nonspeculative) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_read2(%addr : memref<i32>) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  %1 = omp.atomic.read memory_order(xyz) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_read3(%addr : memref<i32>) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read memory_order(acq_rel) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_read4(%addr : memref<i32>) {
+  // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
+  %1 = omp.atomic.read memory_order(release) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_read5(%addr : memref<i32>) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read memory_order(acquire) memory_order(relaxed) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_read6(%addr : memref<i32>) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  %1 = omp.atomic.read hint(speculative) hint(contended) %addr : memref<i32> -> i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write1(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.atomic.write hint(contended, uncontended) %addr : memref<i32>, %val : i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write2(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for atomic writes}}
+  omp.atomic.write memory_order(acq_rel) %addr : memref<i32>, %val : i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write3(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{memory-order must not be acq_rel or acquire for atomic writes}}
+  omp.atomic.write memory_order(acquire) %addr : memref<i32>, %val : i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write4(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.write operation}}
+  omp.atomic.write memory_order(release) memory_order(seq_cst) %addr : memref<i32>, %val : i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write5(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{at most one hint clause can appear on the omp.atomic.write operation}}
+  omp.atomic.write hint(contended) hint(speculative) %addr : memref<i32>, %val : i32
+  return
+}
+
+// -----
+
+func @omp_atomic_write6(%addr : memref<i32>, %val : i32) {
+  // expected-error @below {{attribute 'memory_order' failed to satisfy constraint: MemoryOrderKind Clause}}
+  omp.atomic.write memory_order(xyz) %addr : memref<i32>, %val : i32
+  return
+}

mlir/test/Dialect/OpenMP/ops.mlir

omp.critical(@mutex) hint(uncontended, speculative) {
     omp.terminator
   }
   // CHECK: omp.critical(@{{.*}}) hint(contended, speculative)
   omp.critical(@mutex) hint(speculative, contended) {
     omp.terminator
   }
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+func @omp_atomic_read(%addr : memref<i32>) {
+  // CHECK: %{{.*}} = omp.atomic.read %{{.*}} : memref<i32> -> i32
+  %1 = omp.atomic.read %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(seq_cst) %{{.*}} : memref<i32> -> i32
+  %2 = omp.atomic.read memory_order(seq_cst) %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(acquire) %{{.*}} : memref<i32> -> i32
+  %5 = omp.atomic.read memory_order(acquire) %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(relaxed) %{{.*}} : memref<i32> -> i32
+  %6 = omp.atomic.read memory_order(relaxed) %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read hint(contended, nonspeculative) %{{.*}} : memref<i32> -> i32
+  %7 = omp.atomic.read hint(nonspeculative, contended) %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read memory_order(seq_cst) hint(contended, speculative) %{{.*}} : memref<i32> -> i32
+  %8 = omp.atomic.read hint(speculative, contended) memory_order(seq_cst) %addr : memref<i32> -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+func @omp_atomic_write(%addr : memref<i32>, %val : i32) {
+  // CHECK: omp.atomic.write %{{.*}} : memref<i32>, %{{.*}} : i32
+  omp.atomic.write %addr : memref<i32>, %val : i32
+  // CHECK: omp.atomic.write memory_order(seq_cst) %{{.*}} : memref<i32>, %{{.*}} : i32
+  omp.atomic.write memory_order(seq_cst) %addr : memref<i32>, %val : i32
+  // CHECK: omp.atomic.write memory_order(release) %{{.*}} : memref<i32>, %{{.*}} : i32
+  omp.atomic.write memory_order(release) %addr : memref<i32>, %val : i32
+  // CHECK: omp.atomic.write memory_order(relaxed) %{{.*}} : memref<i32>, %{{.*}} : i32
+  omp.atomic.write memory_order(relaxed) %addr : memref<i32>, %val : i32
+  // CHECK: omp.atomic.write hint(uncontended, speculative) %{{.*}} : memref<i32>, %{{.*}} : i32
+  omp.atomic.write hint(speculative, uncontended) %addr : memref<i32>, %val : i32
+  return
+}