[mlir][OpenMP] Added assemblyFormat for atomic and critical operations

[shraiysh]

Summary

This patch adds assemblyFormat for omp.critical.declare, omp.atomic.read,
omp.atomic.write, omp.atomic.update and omp.atomic.capture.

Also removing those clauses from parseClauses that aren't needed
anymore, thanks to the new assemblyFormats.

[NimishMishra]

Thanks @shraiysh for the patch. I have a few questions. Please find them below.

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

577

I think you have introduced the operations for atomic in an earlier patch. It was merged below I realised this problem. Probably we can fix this here.

According to OpenMP 5.0 specification, (section 2,17.7) atomic read can't have acq_rel or release memory-order clause.

610

Same thing here.

According to OpenMP 5.0 specification, (section 2,17.7) atomic write can't have acq_rel or acquire memory-order clause.

616–624

I do not understand the attribute name change. It doesn't look like $hint and $memory_order are colliding with other names in the critical operation (or other operations in atomic construct which you have updated). Is there a special need for this change?

620

It looks like this updates assembly format for critical construct too. Does it make sense to add it to the patch description? Also is there any need of adding a test case for the same?

643

Same thing here.

According to OpenMP 5.0 specification, (section 2,17.7) atomic update can't have acq_rel or acquire memory-order clause.

mlir/test/Dialect/OpenMP/ops.mlir

571

Just confirming once whether this whitespace is intentional.

614

The general format of these tests looks like the //CHECKs are closer to the statement that generates the IR. Does it make sense to format the atomic tests in the same vein for consistency?

[shraiysh]

Addressed comments. Rebase with main.

[shraiysh]

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

616–624

Yes, oilist adds a unit attribute with the name of the literal associated with a clause. So, hint and memory_order are added by default and will cause collision with same name attributes.

620

Updated the patch description. Tests for this are already in ops.mlir so we don't have to add anything.

mlir/test/Dialect/OpenMP/ops.mlir

571

Yes, the generated printer emits a whitespace here - so to match that, this was updated.

614

I have tried to keep the CHECK for one atomic capture operation right above it so as to have one-one correspondence between atomic.capture ops.

Anyways, I was trying to keep minimal changes to testcases for this patch, because we want same behavior with new format. Best case scenario would be no changes to the tests at all! (We can do it in a separate patch later if you'd like)

[NimishMishra]

Thanks @shraiysh. LGTM.

[shraiysh]

Ping for another approval from someone outside AMD.

Rebase with main.

[shraiysh]

Thanks for the review @rriddle. Rebase with main.

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

def ParallelOp : OpenMP_Op<"parallel", [
   let assemblyFormat = [{
     oilist( `if` `(` $if_expr_var `:` type($if_expr_var) `)`
           | `num_threads` `(` $num_threads_var `:` type($num_threads_var) `)`
           | `allocate` `(`
               custom<AllocateAndAllocator>(
                 $allocate_vars, type($allocate_vars),
                 $allocators_vars, type($allocators_vars)
               ) `)`
-          | `proc_bind` `(` custom<ProcBindKind>($proc_bind_val) `)`
+          | `proc_bind` `(` custom<ClauseAttr>($proc_bind_val) `)`
     ) $region attr-dict
   }];
   let hasVerifier = 1;
 }
 
 def TerminatorOp : OpenMP_Op<"terminator", [Terminator]> {
   let summary = "terminator for OpenMP regions";
   let description = [{
def YieldOp : OpenMP_Op<"yield",
   let description = [{
     "omp.yield" yields SSA values from the OpenMP dialect op region and
     terminates the region. The semantics of how the values are yielded is
     defined by the parent operation.
   }];
 
   let arguments = (ins Variadic<AnyType>:$results);
 
+  let builders = [
+    OpBuilder<(ins), [{ build($_builder, $_state, {}); }]>
+  ];
+
   let assemblyFormat = [{ ( `(` $results^ `:` type($results) `)` )? attr-dict}];
 }
 
 //===----------------------------------------------------------------------===//
 // 2.10.4 taskyield Construct
 //===----------------------------------------------------------------------===//
 
 def TaskyieldOp : OpenMP_Op<"taskyield"> {
def CriticalDeclareOp : OpenMP_Op<"critical.declare", [Symbol]> {
 
   let description = [{
     Declares a named critical section.
 
     The name can be used in critical constructs in the dialect.
   }];
 
   let arguments = (ins SymbolNameAttr:$sym_name,
-                       DefaultValuedAttr<I64Attr, "0">:$hint);
+                       DefaultValuedAttr<I64Attr, "0">:$hint_val);
 
   let assemblyFormat = [{
-    $sym_name custom<SynchronizationHint>($hint) attr-dict
+    $sym_name oilist(`hint` `(` custom<SynchronizationHint>($hint_val) `)`)
+    attr-dict
   }];
   let hasVerifier = 1;
 }
 
 
 def CriticalOp : OpenMP_Op<"critical"> {
   let summary = "critical construct";
   let description = [{

 
 // 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"> {
+def AtomicReadOp : OpenMP_Op<"atomic.read", [AllTypesMatch<["x", "v"]>]> {
 
   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.
 
     `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
     optimization.
 
     `memory_order` indicates the memory ordering behavior of the construct. It
-    can be one of `seq_cst`, `acq_rel`, `release`, `acquire` or `relaxed`.
+    can be one of `seq_cst`, `acquire` or `relaxed`.
   }];
 
   let arguments = (ins OpenMP_PointerLikeType:$x,
                        OpenMP_PointerLikeType:$v,
-                       DefaultValuedAttr<I64Attr, "0">:$hint,
-                       OptionalAttr<MemoryOrderKindAttr>:$memory_order);
-  let hasCustomAssemblyFormat = 1;
+                       DefaultValuedAttr<I64Attr, "0">:$hint_val,
+                       OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
+  let assemblyFormat = [{
+    $v `=` $x 
+    oilist( `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`
+          | `hint` `(` custom<SynchronizationHint>($hint_val) `)`)
+    `:` type($x) attr-dict
+  }];
   let hasVerifier = 1;
 }
 
 def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
 
   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).
 
     `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
     optimization.
 
     `memory_order` indicates the memory ordering behavior of the construct. It
-    can be one of `seq_cst`, `acq_rel`, `release`, `acquire` or `relaxed`.
+    can be one of `seq_cst`, `release` or `relaxed`.
   }];
 
   let arguments = (ins OpenMP_PointerLikeType:$address,
                        AnyType:$value,
-                       DefaultValuedAttr<I64Attr, "0">:$hint,
-                       OptionalAttr<MemoryOrderKindAttr>:$memory_order);
-  let hasCustomAssemblyFormat = 1;
+                       DefaultValuedAttr<I64Attr, "0">:$hint_val,
+                       OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
+  let assemblyFormat = [{
+    $address `=` $value
+    oilist( `hint` `(` custom<SynchronizationHint>($hint_val) `)`
+          | `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`)
+    `:` type($address) `,` type($value)
+    attr-dict
+  }];
   let hasVerifier = 1;
 }
 
 def AtomicUpdateOp : OpenMP_Op<"atomic.update",
                                [SingleBlockImplicitTerminator<"YieldOp">]> {
 
   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 OpenMP
     Standard (OpenMP 5.0, section 2.17.7). It is the address of the variable
     that is being updated. `x` is atomically read/written.
 
     `hint` is the value of hint (as used in the hint clause). It is a compile
     time constant. As the name suggests, this is just a hint for optimization.
 
     `memory_order` indicates the memory ordering behavior of the construct. It
-    can be one of `seq_cst`, `acq_rel`, `release`, `acquire` or `relaxed`.
+    can be one of `seq_cst`, `release` or `relaxed`.
 
     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 arguments = (ins OpenMP_PointerLikeType:$x,
-                       DefaultValuedAttr<I64Attr, "0">:$hint,
-                       OptionalAttr<MemoryOrderKindAttr>:$memory_order);
+                       DefaultValuedAttr<I64Attr, "0">:$hint_val,
+                       OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
   let regions = (region SizedRegion<1>:$region);
-  let hasCustomAssemblyFormat = 1;
+  let assemblyFormat = [{
+    oilist( `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`
+          | `hint` `(` custom<SynchronizationHint>($hint_val) `)`)
+    $x `:` type($x) $region attr-dict
+  }];
   let hasVerifier = 1;
 }
 
 def AtomicCaptureOp : OpenMP_Op<"atomic.capture",
     [SingleBlockImplicitTerminator<"TerminatorOp">]> {
   let summary = "performs an atomic capture";
   let description = [{
     This operation performs an atomic capture.
```
         omp.atomic.read ...
         omp.atomic.write ...
         omp.terminator
       }
     ```
 
   }];
 
-  let arguments = (ins DefaultValuedAttr<I64Attr, "0">:$hint,
-                       OptionalAttr<MemoryOrderKind>:$memory_order);
+  let arguments = (ins DefaultValuedAttr<I64Attr, "0">:$hint_val,
+                       OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
   let regions = (region SizedRegion<1>:$region);
-  let hasCustomAssemblyFormat = 1;
+  let assemblyFormat = [{
+    oilist(`memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`
+          |`hint` `(` custom<SynchronizationHint>($hint_val) `)`)
+    $region attr-dict
+  }];
   let hasVerifier = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // 2.19.5.7 declare reduction Directive
 //===----------------------------------------------------------------------===//
 
 def ReductionDeclareOp : OpenMP_Op<"reduction.declare", [Symbol]> {

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

static void printAllocateAndAllocator(OpAsmPrinter &p, Operation *op,
                                       TypeRange typesAllocator) {
   for (unsigned i = 0; i < varsAllocate.size(); ++i) {
     std::string separator = i == varsAllocate.size() - 1 ? "" : ", ";
     p << varsAllocator[i] << " : " << typesAllocator[i] << " -> ";
     p << varsAllocate[i] << " : " << typesAllocate[i] << separator;
   }
 }
 
+/// Parse a clause attribute (StringEnumAttr)
+template <typename ClauseAttr>
+static ParseResult parseClauseAttr(AsmParser &parser, ClauseAttr &attr) {
+  using ClauseT = decltype(std::declval<ClauseAttr>().getValue());
+  StringRef enumStr;
+  SMLoc loc = parser.getCurrentLocation();
+  if (parser.parseKeyword(&enumStr))
+    return failure();
+  if (Optional<ClauseT> enumValue = symbolizeEnum<ClauseT>(enumStr)) {
+    attr = ClauseAttr::get(parser.getContext(), *enumValue);
+    return success();
+  }
+  return parser.emitError(loc, "invalid clause value: '") << enumStr << "'";
+}
+
+template <typename ClauseAttr>
+void printClauseAttr(OpAsmPrinter &p, Operation *op, ClauseAttr attr) {
+  p << stringifyEnum(attr.getValue());
+}
+
+//===----------------------------------------------------------------------===//
+// Parser and printer for Procbind Clause
+//===----------------------------------------------------------------------===//
+
 ParseResult parseProcBindKind(OpAsmParser &parser,
                               omp::ClauseProcBindKindAttr &procBindAttr) {
   StringRef procBindStr;
   if (parser.parseKeyword(&procBindStr))
     return failure();
   if (auto procBindVal = symbolizeClauseProcBindKind(procBindStr)) {
     procBindAttr =
         ClauseProcBindKindAttr::get(parser.getContext(), *procBindVal);
for (unsigned i = 0; i < linearVarsSize; ++i) {
     p << linearVars[i];
     if (linearStepVars.size() > i)
       p << " = " << linearStepVars[i];
     p << " : " << linearVars[i].getType() << separator;
   }
 }
 
 //===----------------------------------------------------------------------===//
-// Parser and printer for Schedule Clause
+// Parser, printer and verifier for Schedule Clause
 //===----------------------------------------------------------------------===//
 
 static ParseResult
 verifyScheduleModifiers(OpAsmParser &parser,
                         SmallVectorImpl<SmallString<12>> &modifiers) {
   if (modifiers.size() > 2)
     return parser.emitError(parser.getNameLoc()) << " unexpected modifier(s)";
   for (const auto &mod : modifiers) {

 // 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,
-                                            bool parseKeyword = true) {
-  if (parseKeyword && failed(parser.parseOptionalKeyword("hint"))) {
-    hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), 0);
-    return success();
-  }
-
-  if (failed(parser.parseLParen()))
-    return failure();
+                                            IntegerAttr &hintAttr) {
   StringRef hintKeyword;
   int64_t hint = 0;
   do {
     if (failed(parser.parseKeyword(&hintKeyword)))
       return failure();
     if (hintKeyword == "uncontended")
       hint |= 1;
     else if (hintKeyword == "contended")
       hint |= 2;
     else if (hintKeyword == "nonspeculative")
       hint |= 4;
     else if (hintKeyword == "speculative")
       hint |= 8;
     else
       return parser.emitError(parser.getCurrentLocation())
              << hintKeyword << " is not a valid hint";
   } while (succeeded(parser.parseOptionalComma()));
-  if (failed(parser.parseRParen()))
-    return failure();
   hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), hint);
   return success();
 }
 
 /// Prints a Synchronization Hint clause
 static void printSynchronizationHint(OpAsmPrinter &p, Operation *op,
                                      IntegerAttr hintAttr) {
   int64_t hint = hintAttr.getInt();
if (uncontended)
     hints.push_back("uncontended");
   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 << ") ";
 }
 
 /// Verifies a synchronization hint clause
 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 (uncontended && contended)
     return op->emitOpError() << "the hints omp_sync_hint_uncontended and "
                                 "omp_sync_hint_contended cannot be combined";
   if (nonspeculative && speculative)
     return op->emitOpError() << "the hints omp_sync_hint_nonspeculative and "
                                 "omp_sync_hint_speculative cannot be combined.";
   return success();
 }
 
 enum ClauseType {
-  ifClause,
-  numThreadsClause,
-  deviceClause,
-  threadLimitClause,
   allocateClause,
-  procBindClause,
   reductionClause,
   nowaitClause,
   linearClause,
   scheduleClause,
   collapseClause,
   orderClause,
   orderedClause,
-  memoryOrderClause,
-  hintClause,
   COUNT
 };
 
 //===----------------------------------------------------------------------===//
 // Parser for Clause List
 //===----------------------------------------------------------------------===//
 
-/// Parse a clause attribute `(` $value `)`.
-template <typename ClauseAttr>
-static ParseResult parseClauseAttr(AsmParser &parser, OperationState &state,
-                                   StringRef attrName, StringRef name) {
-  using ClauseT = decltype(std::declval<ClauseAttr>().getValue());
-  StringRef enumStr;
-  SMLoc loc = parser.getCurrentLocation();
-  if (parser.parseLParen() || parser.parseKeyword(&enumStr) ||
-      parser.parseRParen())
-    return failure();
-  if (Optional<ClauseT> enumValue = symbolizeEnum<ClauseT>(enumStr)) {
-    auto attr = ClauseAttr::get(parser.getContext(), *enumValue);
-    state.addAttribute(attrName, attr);
-    return success();
-  }
-  return parser.emitError(loc, "invalid ") << name << " kind";
-}
-
 /// Parse a list of clauses. The clauses can appear in any order, but their
 /// operand segment indices are in the same order that they are passed in the
 /// `clauses` list. The operand segments are added over the prevSegments
 
 /// clause-list ::= clause clause-list | empty
-/// clause ::= if | num-threads | allocate | proc-bind | reduction | nowait
-///          | linear | schedule | collapse | order | ordered | inclusive
-/// if ::= `if` `(` ssa-id-and-type `)`
-/// num-threads ::= `num_threads` `(` ssa-id-and-type `)`
+/// clause ::= allocate | reduction | nowait | linear | schedule | collapse
+///          | order | ordered
 /// allocate ::= `allocate` `(` allocate-operand-list `)`
-/// proc-bind ::= `proc_bind` `(` (`master` | `close` | `spread`) `)`
 /// reduction ::= `reduction` `(` reduction-entry-list `)`
 /// nowait ::= `nowait`
 /// linear ::= `linear` `(` linear-list `)`
 /// schedule ::= `schedule` `(` sched-list `)`
 /// collapse ::= `collapse` `(` ssa-id-and-type `)`
 /// order ::= `order` `(` `concurrent` `)`
 /// ordered ::= `ordered` `(` ssa-id-and-type `)`
-/// inclusive ::= `inclusive`
 ///
 /// Note that each clause can only appear once in the clase-list.
 static ParseResult parseClauses(OpAsmParser &parser, OperationState &result,
                                 SmallVectorImpl<ClauseType> &clauses,
                                 SmallVectorImpl<int> &segments) {
 
   // Check done[clause] to see if it has been parsed already
   BitVector done(ClauseType::COUNT, false);
 
   // See pos[clause] to get position of clause in operand segments
   SmallVector<int> pos(ClauseType::COUNT, -1);
 
   // Stores the last parsed clause keyword
   StringRef clauseKeyword;
   StringRef opName = result.name.getStringRef();
 
   // Containers for storing operands, types and attributes for various clauses
-  std::pair<OpAsmParser::OperandType, Type> ifCond;
-  std::pair<OpAsmParser::OperandType, Type> numThreads;
-  std::pair<OpAsmParser::OperandType, Type> device;
-  std::pair<OpAsmParser::OperandType, Type> threadLimit;
-
   SmallVector<OpAsmParser::OperandType> allocates, allocators;
   SmallVector<Type> allocateTypes, allocatorTypes;
 
   ArrayAttr reductions;
   SmallVector<OpAsmParser::OperandType> reductionVars;
   SmallVector<Type> reductionVarTypes;
 
   SmallVector<OpAsmParser::OperandType> linears;
   SmallVector<Type> linearTypes;
   SmallVector<OpAsmParser::OperandType> linearSteps;
 
   SmallString<8> schedule;
   SmallVector<SmallString<12>> modifiers;
   Optional<OpAsmParser::OperandType> scheduleChunkSize;
   Type scheduleChunkType;
 
   // Compute the position of clauses in operand segments
   int currPos = 0;
   for (ClauseType clause : clauses) {
 
     // Skip the following clauses - they do not take any position in operand
     // segments
-    if (clause == procBindClause || clause == nowaitClause ||
-        clause == collapseClause || clause == orderClause ||
-        clause == orderedClause)
+    if (clause == nowaitClause || clause == collapseClause ||
+        clause == orderClause || clause == orderedClause)
       continue;
 
     pos[clause] = currPos++;
 
     // For the following clauses, two positions are reserved in the operand
     // segments
     if (clause == allocateClause || clause == linearClause)
       currPos++;
if (done[clause])
       return parser.emitError(parser.getCurrentLocation())
              << "at most one " << clauseKeyword << " clause can appear on the "
              << opName << " operation";
     done[clause] = true;
     return success();
   };
 
   while (succeeded(parser.parseOptionalKeyword(&clauseKeyword))) {
-    if (clauseKeyword == "if") {
-      if (checkAllowed(ifClause) || parser.parseLParen() ||
-          parser.parseOperand(ifCond.first) ||
-          parser.parseColonType(ifCond.second) || parser.parseRParen())
-        return failure();
-      clauseSegments[pos[ifClause]] = 1;
-    } else if (clauseKeyword == "num_threads") {
-      if (checkAllowed(numThreadsClause) || parser.parseLParen() ||
-          parser.parseOperand(numThreads.first) ||
-          parser.parseColonType(numThreads.second) || parser.parseRParen())
-        return failure();
-      clauseSegments[pos[numThreadsClause]] = 1;
-    } else if (clauseKeyword == "device") {
-      if (checkAllowed(deviceClause) || parser.parseLParen() ||
-          parser.parseOperand(device.first) ||
-          parser.parseColonType(device.second) || parser.parseRParen())
-        return failure();
-      clauseSegments[pos[deviceClause]] = 1;
-    } else if (clauseKeyword == "thread_limit") {
-      if (checkAllowed(threadLimitClause) || parser.parseLParen() ||
-          parser.parseOperand(threadLimit.first) ||
-          parser.parseColonType(threadLimit.second) || parser.parseRParen())
-        return failure();
-      clauseSegments[pos[threadLimitClause]] = 1;
-    } else if (clauseKeyword == "allocate") {
+    if (clauseKeyword == "allocate") {
       if (checkAllowed(allocateClause) || parser.parseLParen() ||
           parseAllocateAndAllocator(parser, allocates, allocateTypes,
                                     allocators, allocatorTypes) ||
           parser.parseRParen())
         return failure();
       clauseSegments[pos[allocateClause]] = allocates.size();
       clauseSegments[pos[allocateClause] + 1] = allocators.size();
-    } else if (clauseKeyword == "proc_bind") {
-      if (checkAllowed(procBindClause) ||
-          parseClauseAttr<ClauseProcBindKindAttr>(parser, result,
-                                                  "proc_bind_val", "proc bind"))
-        return failure();
     } else if (clauseKeyword == "reduction") {
       if (checkAllowed(reductionClause) || parser.parseLParen() ||
           parseReductionVarList(parser, reductionVars, reductionVarTypes,
                                 reductions) ||
           parser.parseRParen())
         return failure();
       clauseSegments[pos[reductionClause]] = reductionVars.size();
     } else if (clauseKeyword == "nowait") {
if (clauseKeyword == "allocate") {
         if (parser.parseAttribute(attr, type) || parser.parseRParen())
           return failure();
       } else {
         // Use 0 to represent no ordered parameter was specified
         attr = parser.getBuilder().getI64IntegerAttr(0);
       }
       result.addAttribute("ordered_val", attr);
     } else if (clauseKeyword == "order") {
-      if (checkAllowed(orderClause) ||
-          parseClauseAttr<ClauseOrderKindAttr>(parser, result, "order_val",
-                                               "order"))
-        return failure();
-    } else if (clauseKeyword == "memory_order") {
-      if (checkAllowed(memoryOrderClause) ||
-          parseClauseAttr<ClauseMemoryOrderKindAttr>(
-              parser, result, "memory_order", "memory order"))
-        return failure();
-    } else if (clauseKeyword == "hint") {
-      IntegerAttr hint;
-      if (checkAllowed(hintClause) ||
-          parseSynchronizationHint(parser, hint, false))
+      ClauseOrderKindAttr order;
+      if (checkAllowed(orderClause) || parser.parseLParen() ||
+          parseClauseAttr<ClauseOrderKindAttr>(parser, order) ||
+          parser.parseRParen())
         return failure();
-      result.addAttribute("hint", hint);
+      result.addAttribute("order_val", order);
     } else {
       return parser.emitError(parser.getNameLoc())
              << clauseKeyword << " is not a valid clause";
     }
   }
 
-  // Add if parameter.
-  if (done[ifClause] && clauseSegments[pos[ifClause]] &&
-      failed(
-          parser.resolveOperand(ifCond.first, ifCond.second, result.operands)))
-    return failure();
-
-  // Add num_threads parameter.
-  if (done[numThreadsClause] && clauseSegments[pos[numThreadsClause]] &&
-      failed(parser.resolveOperand(numThreads.first, numThreads.second,
-                                   result.operands)))
-    return failure();
-
-  // Add device parameter.
-  if (done[deviceClause] && clauseSegments[pos[deviceClause]] &&
-      failed(
-          parser.resolveOperand(device.first, device.second, result.operands)))
-    return failure();
-
-  // Add thread_limit parameter.
-  if (done[threadLimitClause] && clauseSegments[pos[threadLimitClause]] &&
-      failed(parser.resolveOperand(threadLimit.first, threadLimit.second,
-                                   result.operands)))
-    return failure();
-
   // Add allocate parameters.
   if (done[allocateClause] && clauseSegments[pos[allocateClause]] &&
       failed(parser.resolveOperands(allocates, allocateTypes,
                                     allocates[0].location, result.operands)))
     return failure();
 
   // Add allocator parameters.
   if (done[allocateClause] && clauseSegments[pos[allocateClause] + 1] &&
LogicalResult WsLoopOp::verify() {
   return verifyReductionVarList(*this, reductions(), reduction_vars());
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for critical construct (2.17.1)
 //===----------------------------------------------------------------------===//
 
 LogicalResult CriticalDeclareOp::verify() {
-  return verifySynchronizationHint(*this, hint());
+  return verifySynchronizationHint(*this, hint_val());
 }
 
 LogicalResult CriticalOp::verify() {
   if (nameAttr()) {
     SymbolRefAttr symbolRef = nameAttr();
     auto decl = SymbolTable::lookupNearestSymbolFrom<CriticalDeclareOp>(
         *this, symbolRef);
     if (!decl) {
if (!container.ordered_valAttr() ||
                            << "a worksharing-loop region with an ordered "
                            << "clause without parameter present";
   }
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
-// AtomicReadOp
+// Verifier for AtomicReadOp
 //===----------------------------------------------------------------------===//
 
-/// Parser for AtomicReadOp
-///
-/// operation ::= `omp.atomic.read` atomic-clause-list address `->` result-type
-/// address ::= operand `:` type
-ParseResult AtomicReadOp::parse(OpAsmParser &parser, OperationState &result) {
-  OpAsmParser::OperandType x, v;
-  Type addressType;
-  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
-  SmallVector<int> segments;
-
-  if (parser.parseOperand(v) || parser.parseEqual() || parser.parseOperand(x) ||
-      parseClauses(parser, result, clauses, segments) ||
-      parser.parseColonType(addressType) ||
-      parser.resolveOperand(x, addressType, result.operands) ||
-      parser.resolveOperand(v, addressType, result.operands))
-    return failure();
-
-  return success();
-}
-
-void AtomicReadOp::print(OpAsmPrinter &p) {
-  p << " " << v() << " = " << x() << " ";
-  if (auto mo = memory_order())
-    p << "memory_order(" << stringifyClauseMemoryOrderKind(*mo) << ") ";
-  if (hintAttr())
-    printSynchronizationHint(p << " ", *this, hintAttr());
-  p << ": " << x().getType();
-}
-
-/// Verifier for AtomicReadOp
 LogicalResult AtomicReadOp::verify() {
-  if (auto mo = memory_order()) {
+  if (auto mo = memory_order_val()) {
     if (*mo == ClauseMemoryOrderKind::acq_rel ||
         *mo == ClauseMemoryOrderKind::release) {
       return emitError(
           "memory-order must not be acq_rel or release for atomic reads");
     }
   }
   if (x() == v())
     return emitError(
         "read and write must not be to the same location for atomic reads");
-  return verifySynchronizationHint(*this, hint());
+  return verifySynchronizationHint(*this, hint_val());
 }
 
 //===----------------------------------------------------------------------===//
-// AtomicWriteOp
+// Verifier for AtomicWriteOp
 //===----------------------------------------------------------------------===//
 
-/// Parser for AtomicWriteOp
-///
-/// operation ::= `omp.atomic.write` atomic-clause-list operands
-/// operands ::= address `,` value
-/// address ::= operand `:` type
-/// value ::= operand `:` type
-ParseResult AtomicWriteOp::parse(OpAsmParser &parser, OperationState &result) {
-  OpAsmParser::OperandType address, value;
-  Type addrType, valueType;
-  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
-  SmallVector<int> segments;
-
-  if (parser.parseOperand(address) || parser.parseEqual() ||
-      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();
-}
-
-void AtomicWriteOp::print(OpAsmPrinter &p) {
-  p << " " << address() << " = " << value() << " ";
-  if (auto mo = memory_order())
-    p << "memory_order(" << stringifyClauseMemoryOrderKind(*mo) << ") ";
-  if (hintAttr())
-    printSynchronizationHint(p, *this, hintAttr());
-  p << ": " << address().getType() << ", " << value().getType();
-}
-
-/// Verifier for AtomicWriteOp
 LogicalResult AtomicWriteOp::verify() {
-  if (auto mo = memory_order()) {
+  if (auto mo = memory_order_val()) {
     if (*mo == ClauseMemoryOrderKind::acq_rel ||
         *mo == ClauseMemoryOrderKind::acquire) {
       return emitError(
           "memory-order must not be acq_rel or acquire for atomic writes");
     }
   }
-  return verifySynchronizationHint(*this, hint());
+  return verifySynchronizationHint(*this, hint_val());
 }
 
 //===----------------------------------------------------------------------===//
-// AtomicUpdateOp
+// Verifier for AtomicUpdateOp
 //===----------------------------------------------------------------------===//
 
-/// Parser for AtomicUpdateOp
-///
-/// operation ::= `omp.atomic.update` atomic-clause-list ssa-id-and-type region
-ParseResult AtomicUpdateOp::parse(OpAsmParser &parser, OperationState &result) {
-  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
-  SmallVector<int> segments;
-  OpAsmParser::OperandType x, expr;
-  Type xType;
-
-  if (parseClauses(parser, result, clauses, segments) ||
-      parser.parseOperand(x) || parser.parseColon() ||
-      parser.parseType(xType) ||
-      parser.resolveOperand(x, xType, result.operands) ||
-      parser.parseRegion(*result.addRegion()))
-    return failure();
-  return success();
-}
-
-void AtomicUpdateOp::print(OpAsmPrinter &p) {
-  p << " ";
-  if (auto mo = memory_order())
-    p << "memory_order(" << stringifyClauseMemoryOrderKind(*mo) << ") ";
-  if (hintAttr())
-    printSynchronizationHint(p, *this, hintAttr());
-  p << x() << " : " << x().getType();
-  p.printRegion(region());
-}
-
-/// Verifier for AtomicUpdateOp
 LogicalResult AtomicUpdateOp::verify() {
-  if (auto mo = memory_order()) {
+  if (auto mo = memory_order_val()) {
     if (*mo == ClauseMemoryOrderKind::acq_rel ||
         *mo == ClauseMemoryOrderKind::acquire) {
       return emitError(
           "memory-order must not be acq_rel or acquire for atomic updates");
     }
   }
 
   if (region().getNumArguments() != 1)
LogicalResult AtomicUpdateOp::verify() {
   if (yieldOp.results().size() != 1)
     return emitError("only updated value must be returned");
   if (yieldOp.results().front().getType() != region().getArgument(0).getType())
     return emitError("input and yielded value must have the same type");
   return success();
 }
 
 //===----------------------------------------------------------------------===//
-// AtomicCaptureOp
+// Verifier for AtomicCaptureOp
 //===----------------------------------------------------------------------===//
 
-ParseResult AtomicCaptureOp::parse(OpAsmParser &parser,
-                                   OperationState &result) {
-  SmallVector<ClauseType> clauses = {memoryOrderClause, hintClause};
-  SmallVector<int> segments;
-  if (parseClauses(parser, result, clauses, segments) ||
-      parser.parseRegion(*result.addRegion()))
-    return failure();
-  return success();
-}
-
-void AtomicCaptureOp::print(OpAsmPrinter &p) {
-  if (memory_order())
-    p << "memory_order(" << memory_order() << ") ";
-  if (hintAttr())
-    printSynchronizationHint(p, *this, hintAttr());
-  p.printRegion(region());
-}
-
-/// Verifier for AtomicCaptureOp
 LogicalResult AtomicCaptureOp::verify() {
   Block::OpListType &ops = region().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);

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
   // If it has a name, it probably has a hint too.
   if (criticalOp.nameAttr()) {
     // The verifiers in OpenMP Dialect guarentee that all the pointers are
     // non-null
     auto symbolRef = criticalOp.nameAttr().cast<SymbolRefAttr>();
     auto criticalDeclareOp =
         SymbolTable::lookupNearestSymbolFrom<omp::CriticalDeclareOp>(criticalOp,
                                                                      symbolRef);
-    hint = llvm::ConstantInt::get(llvm::Type::getInt32Ty(llvmContext),
-                                  static_cast<int>(criticalDeclareOp.hint()));
+    hint =
+        llvm::ConstantInt::get(llvm::Type::getInt32Ty(llvmContext),
+                               static_cast<int>(criticalDeclareOp.hint_val()));
   }
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createCritical(
       ompLoc, bodyGenCB, finiCB, criticalOp.name().getValueOr(""), hint));
   return success();
 }
 
 /// Returns a reduction declaration that corresponds to the given reduction
 /// operation in the given container. Currently only supports reductions inside

 convertOmpAtomicRead(Operation &opInst, llvm::IRBuilderBase &builder,
                      LLVM::ModuleTranslation &moduleTranslation) {
 
   auto readOp = cast<omp::AtomicReadOp>(opInst);
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
 
-  llvm::AtomicOrdering AO = convertAtomicOrdering(readOp.memory_order());
+  llvm::AtomicOrdering AO = convertAtomicOrdering(readOp.memory_order_val());
   llvm::Value *x = moduleTranslation.lookupValue(readOp.x());
   Type xTy = readOp.x().getType().cast<omp::PointerLikeType>().getElementType();
   llvm::Value *v = moduleTranslation.lookupValue(readOp.v());
   Type vTy = readOp.v().getType().cast<omp::PointerLikeType>().getElementType();
   llvm::OpenMPIRBuilder::AtomicOpValue V = {
       v, moduleTranslation.convertType(vTy), false, false};
   llvm::OpenMPIRBuilder::AtomicOpValue X = {
       x, moduleTranslation.convertType(xTy), false, false};
   builder.restoreIP(ompBuilder->createAtomicRead(ompLoc, X, V, AO));
   return success();
 }
 
 /// Converts an omp.atomic.write operation to LLVM IR.
 static LogicalResult
 convertOmpAtomicWrite(Operation &opInst, llvm::IRBuilderBase &builder,
                       LLVM::ModuleTranslation &moduleTranslation) {
   auto writeOp = cast<omp::AtomicWriteOp>(opInst);
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
-  llvm::AtomicOrdering ao = convertAtomicOrdering(writeOp.memory_order());
+  llvm::AtomicOrdering ao = convertAtomicOrdering(writeOp.memory_order_val());
   llvm::Value *expr = moduleTranslation.lookupValue(writeOp.value());
   llvm::Value *dest = moduleTranslation.lookupValue(writeOp.address());
   llvm::Type *ty = moduleTranslation.convertType(writeOp.value().getType());
   llvm::OpenMPIRBuilder::AtomicOpValue x = {dest, ty, /*isSigned=*/false,
                                             /*isVolatile=*/false};
   builder.restoreIP(ompBuilder->createAtomicWrite(ompLoc, x, expr, ao));
   return success();
 }

mlir/test/Dialect/OpenMP/invalid.mlir

func @inclusive_not_a_clause(%lb : index, %ub : index, %step : index) {
   omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) nowait inclusive {
     omp.yield
   }
 }
 
 // -----
 
 func @order_value(%lb : index, %ub : index, %step : index) {
-  // expected-error @below {{invalid order kind}}
+  // expected-error @below {{invalid clause value: 'default'}}
   omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) order(default) {
     omp.yield
   }
 }
 
 // -----
 
 func @if_not_allowed(%lb : index, %ub : index, %step : index, %bool_var : i1) {
-  // expected-error @below {{if is not a valid clause for the omp.wsloop operation}}
+  // expected-error @below {{if is not a valid clause}}
   omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) if(%bool_var: i1) {
     omp.yield
   }
 }
 
 // -----
 
 func @num_threads_not_allowed(%lb : index, %ub : index, %step : index, %int_var : i32) {
-  // expected-error @below {{num_threads is not a valid clause for the omp.wsloop operation}}
+  // expected-error @below {{num_threads is not a valid clause}}
   omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) num_threads(%int_var: i32) {
     omp.yield
   }
 }
 
 // -----
 
 func @proc_bind_not_allowed(%lb : index, %ub : index, %step : index) {
-  // expected-error @below {{proc_bind is not a valid clause for the omp.wsloop operation}}
+  // expected-error @below {{proc_bind is not a valid clause}}
   omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) proc_bind(close) {
     omp.yield
   }
 }
 
 // -----
 
 // expected-error @below {{op expects initializer region with one argument of the reduction type}}
func @omp_atomic_read1(%x: memref<i32>, %v: memref<i32>) {
   // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined.}}
   omp.atomic.read %v = %x hint(speculative, nonspeculative) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read2(%x: memref<i32>, %v: memref<i32>) {
-  // expected-error @below {{invalid memory order kind}}
+  // expected-error @below {{invalid clause value: 'xyz'}}
   omp.atomic.read %v = %x memory_order(xyz) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read3(%x: memref<i32>, %v: memref<i32>) {
   // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
   omp.atomic.read %v = %x memory_order(acq_rel) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read4(%x: memref<i32>, %v: memref<i32>) {
   // expected-error @below {{memory-order must not be acq_rel or release for atomic reads}}
   omp.atomic.read %v = %x memory_order(release) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read5(%x: memref<i32>, %v: memref<i32>) {
-  // expected-error @below {{at most one memory_order clause can appear on the omp.atomic.read operation}}
+  // expected-error @below {{`memory_order` clause can appear at most once in the expansion of the oilist directive}}
   omp.atomic.read %v = %x memory_order(acquire) memory_order(relaxed) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read6(%x: memref<i32>, %v: memref<i32>) {
-  // expected-error @below {{at most one hint clause can appear on the omp.atomic.read operation}}
+  // expected-error @below {{`hint` clause can appear at most once in the expansion of the oilist directive}}
   omp.atomic.read %v =  %x hint(speculative) hint(contended) : memref<i32>
   return
 }
 
 // -----
 
 func @omp_atomic_read6(%x: memref<i32>, %v: memref<i32>) {
   // expected-error @below {{read and write must not be to the same location for atomic reads}}
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}}
+  // expected-error @below {{`memory_order` clause can appear at most once in the expansion of the oilist directive}}
   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}}
+  // expected-error @below {{`hint` clause can appear at most once in the expansion of the oilist directive}}
   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 {{invalid memory order kind}}
+  // expected-error @below {{invalid clause value: 'xyz'}}
   omp.atomic.write  %addr = %val memory_order(xyz) : memref<i32>, i32
   return
 }
 
 // -----
 
 func @omp_atomic_update1(%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}}
^bb0(%xval: i32):
     %newval = llvm.add %xval, %expr : i32
     omp.yield (%newval : i32)
   }
   return
 }
 
 // -----
 
-// expected-note @below {{prior use here}}
 func @omp_atomic_update5(%x: memref<i32>, %expr: i32) {
-  // expected-error @below {{use of value '%x' expects different type than prior uses: 'i32' vs 'memref<i32>'}}
+  // expected-error @below {{invalid kind of type specified}}
   omp.atomic.update %x : i32 {
   ^bb0(%xval: i32):
     %newval = llvm.add %xval, %expr : i32
     omp.yield (%newval : i32)
   }
   return
 }
 

mlir/test/Dialect/OpenMP/ops.mlir

^bb0(%xval: i1):
     omp.yield(%newval : i1)
   }
   return
 }
 
 // CHECK-LABEL: omp_atomic_capture
 // CHECK-SAME: (%[[v:.*]]: memref<i32>, %[[x:.*]]: memref<i32>, %[[expr:.*]]: i32)
 func @omp_atomic_capture(%v: memref<i32>, %x: memref<i32>, %expr: i32) {
-  // CHECK: omp.atomic.capture{
+  // CHECK: omp.atomic.capture {
   // CHECK-NEXT: omp.atomic.update %[[x]] : memref<i32>
   // CHECK-NEXT: (%[[xval:.*]]: i32):
   // CHECK-NEXT:   %[[newval:.*]] = llvm.add %[[xval]], %[[expr]] : i32
   // CHECK-NEXT:   omp.yield(%[[newval]] : i32)
   // CHECK-NEXT: }
   // CHECK-NEXT: omp.atomic.read %[[v]] = %[[x]] : memref<i32>
-  // CHECK-NEXT: omp.terminator
   // CHECK-NEXT: }
   omp.atomic.capture{
     omp.atomic.update %x : memref<i32> {
     ^bb0(%xval: i32):
       %newval = llvm.add %xval, %expr : i32
       omp.yield(%newval : i32)
     }
     omp.atomic.read %v = %x : memref<i32>
-    omp.terminator
   }
-  // CHECK: omp.atomic.capture{
+  // CHECK: omp.atomic.capture {
   // CHECK-NEXT: omp.atomic.read %[[v]] = %[[x]] : memref<i32>
   // CHECK-NEXT: omp.atomic.update %[[x]] : memref<i32>
   // CHECK-NEXT: (%[[xval:.*]]: i32):
   // CHECK-NEXT:   %[[newval:.*]] = llvm.add %[[xval]], %[[expr]] : i32
   // CHECK-NEXT:   omp.yield(%[[newval]] : i32)
   // CHECK-NEXT: }
-  // CHECK-NEXT: omp.terminator
   // CHECK-NEXT: }
   omp.atomic.capture{
     omp.atomic.read %v = %x : memref<i32>
     omp.atomic.update %x : memref<i32> {
     ^bb0(%xval: i32):
       %newval = llvm.add %xval, %expr : i32
       omp.yield(%newval : i32)
     }
-    omp.terminator
   }
-  // CHECK: omp.atomic.capture{
+  // CHECK: omp.atomic.capture {
   // CHECK-NEXT: omp.atomic.read %[[v]] = %[[x]] : memref<i32>
   // CHECK-NEXT: omp.atomic.write %[[x]] = %[[expr]] : memref<i32>, i32
-  // CHECK-NEXT: omp.terminator
   // CHECK-NEXT: }
   omp.atomic.capture{
     omp.atomic.read %v = %x : memref<i32>
     omp.atomic.write %x = %expr : memref<i32>, i32
-    omp.terminator
   }
   return
 }
 
 // CHECK-LABEL: omp_sectionsop
 func @omp_sectionsop(%data_var1 : memref<i32>, %data_var2 : memref<i32>,
                      %data_var3 : memref<i32>, %redn_var : !llvm.ptr<f32>) {
   // CHECK: omp.sections allocate(%{{.*}} : memref<i32> -> %{{.*}} : memref<i32>)
   "omp.sections" (%data_var1, %data_var1) ({
     // CHECK: omp.terminator
     omp.terminator
   }) {allocate, operand_segment_sizes = dense<[0,1,1]> : vector<3xi32>} : (memref<i32>, memref<i32>) -> ()