[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

515

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

515

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

1285

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

1323

Is the order of hint and memory_order different from Read?

mlir/test/Dialect/OpenMP/invalid.mlir

379

should we have tests for allowed once clauses?

474

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

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

513

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

1246

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

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

513

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

1246

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

[shraiysh]

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

1246

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

clang/lib/CodeGen/CGStmtOpenMP.cpp

static void emitOMPAtomicExpr(CodeGenFunction &CGF, OpenMPClauseKind Kind,
   case OMPC_notinbranch:
   case OMPC_link:
   case OMPC_use:
   case OMPC_novariants:
   case OMPC_nocontext:
   case OMPC_filter:
   case OMPC_when:
   case OMPC_adjust_args:
   case OMPC_append_args:
+  case OMPC_memory_order:
     llvm_unreachable("Clause is not allowed in 'omp atomic'.");
   }
 }
 
 void CodeGenFunction::EmitOMPAtomicDirective(const OMPAtomicDirective &S) {
   llvm::AtomicOrdering AO = llvm::AtomicOrdering::Monotonic;
   bool MemOrderingSpecified = false;
   if (S.getSingleClause<OMPSeqCstClause>()) {

flang/lib/Semantics/check-omp-structure.cpp

 CHECK_SIMPLE_CLAUSE(Init, OMPC_init)
 CHECK_SIMPLE_CLAUSE(Use, OMPC_use)
 CHECK_SIMPLE_CLAUSE(Novariants, OMPC_novariants)
 CHECK_SIMPLE_CLAUSE(Nocontext, OMPC_nocontext)
 CHECK_SIMPLE_CLAUSE(Filter, OMPC_filter)
 CHECK_SIMPLE_CLAUSE(When, OMPC_when)
 CHECK_SIMPLE_CLAUSE(AdjustArgs, OMPC_adjust_args)
 CHECK_SIMPLE_CLAUSE(AppendArgs, OMPC_append_args)
+CHECK_SIMPLE_CLAUSE(MemoryOrder, OMPC_memory_order)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
 CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
 CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
 

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 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";
 }

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

 // 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);
enum ClauseType {
   reductionClause,
   nowaitClause,
   linearClause,
   scheduleClause,
   collapseClause,
   orderClause,
   orderedClause,
   inclusiveClause,
+  memoryOrderClause,
+  hintClause,
   COUNT
 };
 
 //===----------------------------------------------------------------------===//
 // Parser for Clause List
 //===----------------------------------------------------------------------===//
 
 /// Parse a list of clauses. The clauses can appear in any order, but their
if (clauseKeyword == "if") {
         return failure();
       auto attr = parser.getBuilder().getStringAttr(order);
       result.addAttribute("order", attr);
     } else if (clauseKeyword == "inclusive") {
       if (checkAllowed(inclusiveClause))
         return failure();
       auto attr = UnitAttr::get(parser.getBuilder().getContext());
       result.addAttribute("inclusive", attr);
+    } else if (clauseKeyword == "memory_order") {
+      StringRef memoryOrder;
+      if (checkAllowed(memoryOrderClause) || parser.parseLParen() ||
+          parser.parseKeyword(&memoryOrder) || parser.parseRParen())
+        return failure();
+      result.addAttribute("memory_order",
+                          parser.getBuilder().getStringAttr(memoryOrder));
+    } else if (clauseKeyword == "hint") {
+      IntegerAttr hint;
+      if (checkAllowed(hintClause) ||
+          parseSynchronizationHint(parser, hint, false))
+        return failure();
+      result.addAttribute("hint", hint);
     } else {
       return parser.emitError(parser.getNameLoc())
              << clauseKeyword << " is not a valid clause";
     }
   }
 
   // Add if parameter.
   if (done[ifClause] && clauseSegments[pos[ifClause]] &&
if (!container.ordered_valAttr() ||
       return op.emitOpError() << "ordered region must be closely nested inside "
                               << "a worksharing-loop region with an ordered "
                               << "clause without parameter present";
   }
 
   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;
+  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
+  SmallVector<int> segments;
+
+  if (parser.parseOperand(address) ||
+      parseClauses(parser, result, clauses, segments) ||
+      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 << " " << op.address() << " ";
+  if (op.memory_order())
+    p << "memory_order(" << op.memory_order().getValue() << ") ";
+  if (op.hintAttr())
+    printSynchronizationHint(p << " ", op, op.hintAttr());
+  p << ": " << op.address().getType() << " -> " << 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;
+  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
+  SmallVector<int> segments;
+
+  if (parser.parseOperand(address) || parser.parseComma() ||
+      parser.parseOperand(value) ||
+      parseClauses(parser, result, clauses, segments) ||
+      parser.parseColonType(addrType) || parser.parseComma() ||
+      parser.parseType(valueType) ||
+      parser.resolveOperand(address, addrType, result.operands) ||
+      parser.resolveOperand(value, valueType, result.operands))
+    return failure();
+  return success();
+}
+
+/// Printer for AtomicWriteOp
+static void printAtomicWriteOp(OpAsmPrinter &p, AtomicWriteOp op) {
+  p << " " << op.address() << ", " << op.value() << " ";
+  if (op.memory_order())
+    p << "memory_order(" << op.memory_order() << ") ";
+  if (op.hintAttr())
+    printSynchronizationHint(p, op, op.hintAttr());
+  p << ": " << op.address().getType() << ", " << 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

func @omp_ordered5(%arg1 : i32, %arg2 : i32, %arg3 : i32, %vec0 : i64, %vec1 : i64) -> () {
   omp.wsloop (%0) : i32 = (%arg1) to (%arg2) step (%arg3) ordered(1) {
     // expected-error @below {{number of variables in depend clause does not match number of iteration variables in the doacross loop}}
     omp.ordered depend_type("dependsource") depend_vec(%vec0, %vec1 : i64, i64) {num_loops_val = 2 : i64}
 
     omp.yield
   }
   return
 }
+
+// -----
+
+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 %addr hint(speculative, nonspeculative) : 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 %addr memory_order(xyz) : 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 %addr memory_order(acq_rel) : 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 %addr memory_order(release) : 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 %addr memory_order(acquire) memory_order(relaxed) : 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  %addr hint(speculative) hint(contended) : 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  %addr, %val hint(contended, uncontended) : memref<i32>, 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  %addr, %val memory_order(acq_rel) : memref<i32>, 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  %addr, %val memory_order(acquire) : memref<i32>, 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  %addr, %val memory_order(release) memory_order(seq_cst) : memref<i32>, 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  %addr, %val hint(contended) hint(speculative) : memref<i32>, 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  %addr, %val memory_order(xyz) : memref<i32>, i32
+  return
+}

mlir/test/Dialect/OpenMP/ops.mlir

omp.wsloop (%0) : i32 = (%arg1) to (%arg2) step (%arg3) ordered(2) {
     // CHECK: omp.ordered depend_type("dependsource") depend_vec(%{{.*}}, %{{.*}} : i64, i64) {num_loops_val = 2 : i64}
     omp.ordered depend_type("dependsource") depend_vec(%vec0, %vec1 : i64, i64) {num_loops_val = 2 : i64}
 
     omp.yield
   }
 
   return
 }
+
+// CHECK-LABEL: omp_atomic_read
+// CHECK-SAME: (%[[ADDR:.*]]: memref<i32>)
+func @omp_atomic_read(%addr : memref<i32>) {
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] : memref<i32> -> i32
+  %1 = omp.atomic.read %addr : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) : memref<i32> -> i32
+  %2 = omp.atomic.read %addr memory_order(seq_cst) : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(acquire) : memref<i32> -> i32
+  %5 = omp.atomic.read %addr memory_order(acquire) : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(relaxed) : memref<i32> -> i32
+  %6 = omp.atomic.read %addr memory_order(relaxed) : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] hint(contended, nonspeculative) : memref<i32> -> i32
+  %7 = omp.atomic.read %addr hint(nonspeculative, contended) : memref<i32> -> i32
+  // CHECK: %{{.*}} = omp.atomic.read %[[ADDR]] memory_order(seq_cst) hint(contended, speculative) : memref<i32> -> i32
+  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) : memref<i32> -> i32
+  return
+}
+
+// CHECK-LABEL: omp_atomic_write
+// CHECK-SAME: (%[[ADDR:.*]]: memref<i32>, %[[VAL:.*]]: i32)
+func @omp_atomic_write(%addr : memref<i32>, %val : i32) {
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] : memref<i32>, i32
+  omp.atomic.write %addr, %val : memref<i32>, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(seq_cst) : memref<i32>, i32
+  omp.atomic.write %addr, %val memory_order(seq_cst) : memref<i32>, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(release) : memref<i32>, i32
+  omp.atomic.write %addr, %val memory_order(release) : memref<i32>, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] memory_order(relaxed) : memref<i32>, i32
+  omp.atomic.write %addr, %val memory_order(relaxed) : memref<i32>, i32
+  // CHECK: omp.atomic.write %[[ADDR]], %[[VAL]] hint(uncontended, speculative) : memref<i32>, i32
+  omp.atomic.write %addr, %val hint(speculative, uncontended) : memref<i32>, i32
+  return
+}