[MLIR][OpenMP] Added parseClauses

[shraiysh]

Summary

Code reorganized in OpenMPDialect.cpp to have all functions corresponding to an operation together.

Added parseClauses function to avoid code duplication while parsing clauses in OpenMP operations. Also added printers and verifiers for clauses, which are being used for multiple operations.

[clementval]

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

203

Usually you don't need to specify the number of inlined elements unless you have a good reason for it. Same for the other SmallVector below.

[shraiysh]

Addressed comments

[clementval]

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

238

I don't see any use of the allowRepeat in the clauses below. Is that correct. Clauses like private and other data related clauses are usually allowed multiple times.

[shraiysh]

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

238

Yes, that is correct. I have not used it anywhere. Earlier an allowedOnce function was used to make sure that there is only one private clause (similarly for others). There are a few invalid.mlir testcases for this too. Personally, I have no preference for either because they can always be clubbed into one. Please let me know if I should change this to allow repetition.

[shraiysh]

Ping!

[Meinersbur]

Is there a reason that WsLoopOp has been moved? It makes it harder to see the differences.

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

193

[style] LLVM's coding style does bit use almost-always-auto

195–197

236

[suggestion] This enum and the parsing of the clauses looks like something that could be generated from the .td. No idea whether it is worth it, but would allow to just add another clause into the td and re-use one of the parsers, e.g. a simple int argument.

[clementval]

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

236

+1

[shraiysh]

Thank you for the review Michael.

The reason for moving the builders and verifier for WsLoopOp is that earlier the parser, printer, verifier and builders for WsLoopOp were all scattered throughout the file. Now they are together. It might be easier to review the file separately, instead of the diff. Here is a brief structure of the file now:

// Parsers and printers for individual clauses...
//
// The function parseClauses.
//
// ParallelOp
//    parseParallelOp calls parseClauses
//
// WsLoopOp
//    parseWsLoopOp calls parseClauses
//
// ReductionOp
//
// CriticalOp

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

236

Thanks for the suggestion Michael and Valentin. I wanted to make sure if I understood it properly.

1. The clauses should be defined in the OpenMPOps.td file and parsers for clause-list in directives should be generated using TableGen (by adding a function in DirectiveCommonGen.cpp).
2. The parsers for individual clauses like one with a simple int argument will be defined in OpenMPDialect.cpp, which can then be re-used for other clauses too.

Is that correct?

[clementval]

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

236

The clause are already defined in the file llvm/include/llvm/Frontend/OpenMP/OMP.td and they should not be defined anywhere else if possible.
The DirectiveCommonGen.cpp is the right place to generate code for MLIR related files from OMP.td.

[shraiysh]

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

236

Thanks Valentin. The generation of these parsers will require the number of variadic operands for every clause, because they have to be added to the operand_segment_sizes attribute. I think that it has to be added as another field to the clauses in OMP.td as mlirSegments. Any suggestions for better alternatives to this would be helpful.

[kiranchandramohan]

In D110903#3060732, @shraiysh wrote:

Thank you for the review Michael.

The reason for moving the builders and verifier for WsLoopOp is that earlier the parser, printer, verifier and builders for WsLoopOp were all scattered throughout the file. Now they are together. It might be easier to review the file separately, instead of the diff. Here is a brief structure of the file now:

// Parsers and printers for individual clauses...
//
// The function parseClauses.
//
// ParallelOp
//    parseParallelOp calls parseClauses
//
// WsLoopOp
//    parseWsLoopOp calls parseClauses
//
// ReductionOp
//
// CriticalOp

Would it be possible to retain the existing ordering and move to the new ordering in a separate NFC patch which you can submit without review? The old ordering will help to review.

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

236

In my personal opinion, let us do autogen in a separate patch after winding up the discussion on extending ODS.

[shraiysh]

Preserving old ordering for easier review. Addressed comments.

[shraiysh]

Fixed Formatting error

[shraiysh]

Rebase with main

[shraiysh]

Ping!

[kiranchandramohan]

In D110903#3069671, @shraiysh wrote:

Ping!

I am reviewing this patch today.

[peixin]

Nice reorganization.

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

97

Prints -> Print?

151

Prints -> Print?

178–179

Why not move printDataVars out? Maybe adding two static functions printDataVars and printOperandAndTypeList is better?

231

It seems that op is not used.

286

"op" is not used.

503

Would it be better to use switch-case statement instead of if-else statement?

656–681

rection -> reduction.

769

Delete the line 764. Declare the variable when it is used.
-> int numIVs = static_cast<int>(ivs.size());

770

Move the line 764 to here.

798

Delte the blank line 802 and 804.

816–817

printDataVars can be moved out. It's the same as the one above.

[kiranchandramohan]

Looks good. Thanks for this re-organization. A few minor comments.

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

326

Is this assert necessary here? This is caught by the verifier right?

656–681

Nit: rection -> reduction

822

Nit: Can the printing of linear, schedule be moved into the printLinearClause or printScheduleClause function? Or alternatively, can the printing of reduction be moved out of printReductionVarList?

mlir/test/Dialect/OpenMP/invalid.mlir

4

Why is this shortened?

[shraiysh]

Thanks for the reviews @peixin @kiranchandramohan . Addressed comments.

@peixin I have mentioned the motivation for putting the op argument in some functions. Please let me know if they should be removed.

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

231

The motivation for doing this was to allow using custom<LinearClause> in assemblyFormat (if required) later. It makes the prototypes of print**Clause functions uniform. Let me know if you want me to remove the argument op.

286

Same reason. Let me know if this needs to be removed.

326

Yes, that is correct. Removed it. Thanks for pointing this out.

503

Normal switch case is not supported on StringRefs. I am not aware of another way to switch case cleanly on strings. If there is a way, please let me know, I will modify the code accordingly.

mlir/test/Dialect/OpenMP/invalid.mlir

4

This is because invalid is not a recognized clause. If it were simply not allowed for omp.parallel but still a valid clause, the error would be the same as before.

[peixin]

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

231

If so, no need to remove it. Maybe add one comment to describe it?

503

Got it. Keep the current style.

[kiranchandramohan]

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

231

If it results in an unused warning then some builders/CI will fail. It seems MLIR is built with -Wno-unused-parameter and hence there is no Warning.

mlir/test/Dialect/OpenMP/invalid.mlir

4

I might be missing a point here, the error that is emitted seems to be the same as before.

error: custom op 'omp.parallel' invalid is not a valid clause for the omp.parallel operation
  omp.parallel invalid {
  ^

[shraiysh]

Thanks for the comments @kiranchandramohan @peixin.

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

231

Oh okay then, I will remove unused parameters.

mlir/test/Dialect/OpenMP/invalid.mlir

4

Hmm this is weird, it doesn't seem to give this error in my build. There are two errors,

• invalid is supposed to go to final else in the while loop (inside parseClauses) and emit the error, invalid is not a valid clause.
• The other case will be when lastprivate is used with ParallelOp (which emits the error from checkAllowed). That should give the error lastprivate is not a valid clause for the omp.parallel operation.

Should I make them the same message to avoid such confusion?

[kiranchandramohan]

LGTM. Thanks once again for this cleanup.

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

231

Nit: Please remove. If required for assembly format, we can re-introduce.

mlir/test/Dialect/OpenMP/invalid.mlir

4

Sorry, this was an issue in my build. I was building flang which does not build mlir-opt. Ignore.

[peixin]

LGTM.

[shraiysh]

Thanks for the review @kiranchandramohan @peixin. I will land this now.

[peixin]

@shraiysh This patch seems to make the test cases in https://reviews.llvm.org/D110015 failed. The following MLIR can be generated from the fortran code using bbc on fir-dev.

subroutine test()
  integer:: i, N = 10
  !$omp do ordered(1)
  do i = 2, N
  end do
  !$omp end do
end subroutine

func @_QPtest() {
  %c2_i32 = constant 2 : i32
  %c1_i32 = constant 1 : i32
  %0 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFtestEi"}
  %1 = fir.address_of(@_QFtestEn) : !fir.ref<i32>
  %2 = fir.load %1 : !fir.ref<i32>
  omp.wsloop (%arg0) : i32 = (%c2_i32) to (%2) step (%c1_i32) ordered(1) inclusive {
    omp.yield
  }
  return
}
fir.global internal @_QFtestEn : i32 {
  %c10_i32 = constant 10 : i32
  fir.has_value %c10_i32 : i32
}

After this PR, the inclusive attribute in the above MLIR seems to be an invalid clause. Do I miss something or understand it incorrectly?

[peixin]

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

795

Is inclusive missed here?

[shraiysh]

In D110903#3076888, @peixin wrote:

@shraiysh This patch seems to make the test cases in https://reviews.llvm.org/D110015 failed. The following MLIR can be generated from the fortran code using bbc on fir-dev.

subroutine test()
  integer:: i, N = 10
  !$omp do ordered(1)
  do i = 2, N
  end do
  !$omp end do
end subroutine

func @_QPtest() {
  %c2_i32 = constant 2 : i32
  %c1_i32 = constant 1 : i32
  %0 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFtestEi"}
  %1 = fir.address_of(@_QFtestEn) : !fir.ref<i32>
  %2 = fir.load %1 : !fir.ref<i32>
  omp.wsloop (%arg0) : i32 = (%c2_i32) to (%2) step (%c1_i32) ordered(1) inclusive {
    omp.yield
  }
  return
}
fir.global internal @_QFtestEn : i32 {
  %c10_i32 = constant 10 : i32
  fir.has_value %c10_i32 : i32
}

After this PR, the inclusive attribute in the above MLIR seems to be an invalid clause. Do I miss something or understand it incorrectly?

Ah! Sorry for this. It is a small error with this patch. I have marked it as an inline comment. I will submit a differential (along with the tests in llvm/main) correcting this error in sometime.

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

792

There should be inclusiveClause in this list. It is not.

[peixin]

In D110903#3076892, @shraiysh wrote:

Ah! Sorry for this. It is a small error with this patch. I have marked it as an inline comment. I will submit a differential (along with the tests in llvm/main) correcting this error in sometime.

Thanks @shraiysh for the confirmation.

@kiranchandramohan Should I revert https://reviews.llvm.org/rGdc2be87ecf10f2f1cf05f638a72256387c78f1c1 now and land it again after this bug is fixed since current ninja-check-mlir have three failed test cases?

[shraiysh]

If it is okay with you, I will check those test cases while pushing my fix. I will rebase with latest main (along with https://reviews.llvm.org/D110015 ) and run tests to avoid such issues. We might not have to revert it, but I understand if you want to revert it.

[peixin]

@shraiysh Sure. Please go ahead and check those test cases with your fix. Please give this fix high priority since current llvm-project cannot pass CI successfully since our patches.

[shraiysh]

Yes, I have added the revision D112198. Once the builds pass, and the reviewers accept it, I will land it. Once again, apologies for this!

[clementval]

In D110903#3076898, @peixin wrote:

In D110903#3076892, @shraiysh wrote:

Ah! Sorry for this. It is a small error with this patch. I have marked it as an inline comment. I will submit a differential (along with the tests in llvm/main) correcting this error in sometime.

Thanks @shraiysh for the confirmation.

@kiranchandramohan Should I revert https://reviews.llvm.org/rGdc2be87ecf10f2f1cf05f638a72256387c78f1c1 now and land it again after this bug is fixed since current ninja-check-mlir have three failed test cases?

In this case you should revert your patch if you cannot fix it directly. There is no problem to land it again later when the problem is fixed.

[peixin]

In D110903#3077076, @clementval wrote:

In this case you should revert your patch if you cannot fix it directly. There is no problem to land it again later when the problem is fixed.

@clementval Got it. Thanks for the explanations.

[clementval]

In D110903#3077084, @peixin wrote:

In D110903#3077076, @clementval wrote:

In this case you should revert your patch if you cannot fix it directly. There is no problem to land it again later when the problem is fixed.

@clementval Got it. Thanks for the explanations.

I know it's not always easy since some buildbot have quite some delays between the commit and the actual build. Anyway if it's really a blocker someone will revert it for you.

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

let arguments = (ins Variadic<IntLikeType>:$lowerBound,
              Variadic<IntLikeType>:$upperBound,
              Variadic<IntLikeType>:$step,
              Variadic<AnyType>:$private_vars,
              Variadic<AnyType>:$firstprivate_vars,
              Variadic<AnyType>:$lastprivate_vars,
              Variadic<AnyType>:$linear_vars,
              Variadic<AnyType>:$linear_step_vars,
              Variadic<OpenMP_PointerLikeType>:$reduction_vars,
-             OptionalAttr<TypedArrayAttrBase<SymbolRefAttr,
-                            "array of symbol references">>:$reductions,
+             OptionalAttr<SymbolRefArrayAttr>:$reductions,
              OptionalAttr<ScheduleKind>:$schedule_val,
              Optional<AnyType>:$schedule_chunk_var,
              Confined<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$collapse_val,
              UnitAttr:$nowait,
              Confined<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$ordered_val,
              OptionalAttr<OrderKind>:$order_val,
              UnitAttr:$inclusive);
 

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"
ParallelOp::build(
       builder, state, /*if_expr_var=*/nullptr, /*num_threads_var=*/nullptr,
       /*default_val=*/nullptr, /*private_vars=*/ValueRange(),
       /*firstprivate_vars=*/ValueRange(), /*shared_vars=*/ValueRange(),
       /*copyin_vars=*/ValueRange(), /*allocate_vars=*/ValueRange(),
       /*allocators_vars=*/ValueRange(), /*proc_bind_val=*/nullptr);
   state.addAttributes(attributes);
 }
 
+//===----------------------------------------------------------------------===//
+// Parser and printer for Operand and type list
+//===----------------------------------------------------------------------===//
+
 /// Parse a list of operands with types.
 ///
 /// operand-and-type-list ::= `(` ssa-id-and-type-list `)`
 /// ssa-id-and-type-list ::= ssa-id-and-type |
 ///                          ssa-id-and-type `,` ssa-id-and-type-list
 /// ssa-id-and-type ::= ssa-id `:` type
 static ParseResult
 parseOperandAndTypeList(OpAsmParser &parser,
                         SmallVectorImpl<OpAsmParser::OperandType> &operands,
                         SmallVectorImpl<Type> &types) {
   return parser.parseCommaSeparatedList(
       OpAsmParser::Delimiter::Paren, [&]() -> ParseResult {
         OpAsmParser::OperandType operand;
         Type type;
         if (parser.parseOperand(operand) || parser.parseColonType(type))
           return failure();
         operands.push_back(operand);
         types.push_back(type);
         return success();
       });
 }
 
+/// Print an operand and type list with parentheses
+static void printOperandAndTypeList(OpAsmPrinter &p, OperandRange operands) {
+  p << "(";
+  llvm::interleaveComma(
+      operands, p, [&](const Value &v) { p << v << " : " << v.getType(); });
+  p << ") ";
+}
+
+/// Print data variables corresponding to a data-sharing clause `name`
+static void printDataVars(OpAsmPrinter &p, OperandRange operands,
+                          StringRef name) {
+  if (operands.size()) {
+    p << name;
+    printOperandAndTypeList(p, operands);
+  }
+}
+
+//===----------------------------------------------------------------------===//
+// Parser and printer for Allocate Clause
+//===----------------------------------------------------------------------===//
+
 /// Parse an allocate clause with allocators and a list of operands with types.
 ///
-/// operand-and-type-list ::= `(` allocate-operand-list `)`
+/// allocate ::= `allocate` `(` allocate-operand-list `)`
 /// allocate-operand-list :: = allocate-operand |
 ///                            allocator-operand `,` allocate-operand-list
 /// allocate-operand :: = ssa-id-and-type -> ssa-id-and-type
 /// ssa-id-and-type ::= ssa-id `:` type
 static ParseResult parseAllocateAndAllocator(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::OperandType> &operandsAllocate,
     SmallVectorImpl<Type> &typesAllocate,
return parser.parseCommaSeparatedList(
           return failure();
 
         operandsAllocate.push_back(operand);
         typesAllocate.push_back(type);
         return success();
       });
 }
 
+/// Print allocate clause
+static void printAllocateAndAllocator(OpAsmPrinter &p,
+                                      OperandRange varsAllocate,
+                                      OperandRange varsAllocator) {
+  if (varsAllocate.empty())
+    return;
+
+  p << "allocate(";
+  for (unsigned i = 0; i < varsAllocate.size(); ++i) {
+    std::string separator = i == varsAllocate.size() - 1 ? ") " : ", ";
+    p << varsAllocator[i] << " : " << varsAllocator[i].getType() << " -> ";
+    p << varsAllocate[i] << " : " << varsAllocate[i].getType() << separator;
+  }
+}
+
 static LogicalResult verifyParallelOp(ParallelOp op) {
   if (op.allocate_vars().size() != op.allocators_vars().size())
     return op.emitError(
         "expected equal sizes for allocate and allocator variables");
   return success();
 }
 
 static void printParallelOp(OpAsmPrinter &p, ParallelOp op) {
+  p << " ";
   if (auto ifCond = op.if_expr_var())
-    p << " if(" << ifCond << " : " << ifCond.getType() << ")";
+    p << "if(" << ifCond << " : " << ifCond.getType() << ") ";
 
   if (auto threads = op.num_threads_var())
-    p << " num_threads(" << threads << " : " << threads.getType() << ")";
+    p << "num_threads(" << threads << " : " << threads.getType() << ") ";
 
-  // Print private, firstprivate, shared and copyin parameters
-  auto printDataVars = [&p](StringRef name, OperandRange vars) {
-    if (vars.size()) {
-      p << " " << name << "(";
-      for (unsigned i = 0; i < vars.size(); ++i) {
-        std::string separator = i == vars.size() - 1 ? ")" : ", ";
-        p << vars[i] << " : " << vars[i].getType() << separator;
-      }
-    }
-  };
-
-  // Print allocator and allocate parameters
-  auto printAllocateAndAllocator = [&p](OperandRange varsAllocate,
-                                        OperandRange varsAllocator) {
-    if (varsAllocate.empty())
-      return;
-
-    p << " allocate(";
-    for (unsigned i = 0; i < varsAllocate.size(); ++i) {
-      std::string separator = i == varsAllocate.size() - 1 ? ")" : ", ";
-      p << varsAllocator[i] << " : " << varsAllocator[i].getType() << " -> ";
-      p << varsAllocate[i] << " : " << varsAllocate[i].getType() << separator;
-    }
-  };
-
-  printDataVars("private", op.private_vars());
-  printDataVars("firstprivate", op.firstprivate_vars());
-  printDataVars("shared", op.shared_vars());
-  printDataVars("copyin", op.copyin_vars());
-  printAllocateAndAllocator(op.allocate_vars(), op.allocators_vars());
+  printDataVars(p, op.private_vars(), "private");
+  printDataVars(p, op.firstprivate_vars(), "firstprivate");
+  printDataVars(p, op.shared_vars(), "shared");
+  printDataVars(p, op.copyin_vars(), "copyin");
+  printAllocateAndAllocator(p, op.allocate_vars(), op.allocators_vars());
 
   if (auto def = op.default_val())
-    p << " default(" << def->drop_front(3) << ")";
+    p << "default(" << def->drop_front(3) << ") ";
 
   if (auto bind = op.proc_bind_val())
-    p << " proc_bind(" << bind << ")";
+    p << "proc_bind(" << bind << ") ";
 
   p.printRegion(op.getRegion());
 }
 
-/// Emit an error if the same clause is present more than once on an operation.
-static ParseResult allowedOnce(OpAsmParser &parser, StringRef clause,
-                               StringRef operation) {
-  return parser.emitError(parser.getNameLoc())
-         << " at most one " << clause << " clause can appear on the "
-         << operation << " operation";
-}
-
-/// Parses a parallel operation.
-///
-/// operation ::= `omp.parallel` clause-list
-/// clause-list ::= clause | clause clause-list
-/// clause ::= if | numThreads | private | firstprivate | shared | copyin |
-///            default | procBind
-/// if ::= `if` `(` ssa-id `)`
-/// numThreads ::= `num_threads` `(` ssa-id-and-type `)`
-/// private ::= `private` operand-and-type-list
-/// firstprivate ::= `firstprivate` operand-and-type-list
-/// shared ::= `shared` operand-and-type-list
-/// copyin ::= `copyin` operand-and-type-list
-/// allocate ::= `allocate` operand-and-type `->` operand-and-type-list
-/// default ::= `default` `(` (`private` | `firstprivate` | `shared` | `none`)
-/// procBind ::= `proc_bind` `(` (`master` | `close` | `spread`) `)`
-///
-/// Note that each clause can only appear once in the clase-list.
-static ParseResult parseParallelOp(OpAsmParser &parser,
-                                   OperationState &result) {
-  std::pair<OpAsmParser::OperandType, Type> ifCond;
-  std::pair<OpAsmParser::OperandType, Type> numThreads;
-  SmallVector<OpAsmParser::OperandType, 4> privates;
-  SmallVector<Type, 4> privateTypes;
-  SmallVector<OpAsmParser::OperandType, 4> firstprivates;
-  SmallVector<Type, 4> firstprivateTypes;
-  SmallVector<OpAsmParser::OperandType, 4> shareds;
-  SmallVector<Type, 4> sharedTypes;
-  SmallVector<OpAsmParser::OperandType, 4> copyins;
-  SmallVector<Type, 4> copyinTypes;
-  SmallVector<OpAsmParser::OperandType, 4> allocates;
-  SmallVector<Type, 4> allocateTypes;
-  SmallVector<OpAsmParser::OperandType, 4> allocators;
-  SmallVector<Type, 4> allocatorTypes;
-  std::array<int, 8> segments{0, 0, 0, 0, 0, 0, 0, 0};
-  StringRef keyword;
-  bool defaultVal = false;
-  bool procBind = false;
-
-  const int ifClausePos = 0;
-  const int numThreadsClausePos = 1;
-  const int privateClausePos = 2;
-  const int firstprivateClausePos = 3;
-  const int sharedClausePos = 4;
-  const int copyinClausePos = 5;
-  const int allocateClausePos = 6;
-  const int allocatorPos = 7;
-  const StringRef opName = result.name.getStringRef();
-
-  while (succeeded(parser.parseOptionalKeyword(&keyword))) {
-    if (keyword == "if") {
-      // Fail if there was already another if condition.
-      if (segments[ifClausePos])
-        return allowedOnce(parser, "if", opName);
-      if (parser.parseLParen() || parser.parseOperand(ifCond.first) ||
-          parser.parseColonType(ifCond.second) || parser.parseRParen())
-        return failure();
-      segments[ifClausePos] = 1;
-    } else if (keyword == "num_threads") {
-      // Fail if there was already another num_threads clause.
-      if (segments[numThreadsClausePos])
-        return allowedOnce(parser, "num_threads", opName);
-      if (parser.parseLParen() || parser.parseOperand(numThreads.first) ||
-          parser.parseColonType(numThreads.second) || parser.parseRParen())
-        return failure();
-      segments[numThreadsClausePos] = 1;
-    } else if (keyword == "private") {
-      // Fail if there was already another private clause.
-      if (segments[privateClausePos])
-        return allowedOnce(parser, "private", opName);
-      if (parseOperandAndTypeList(parser, privates, privateTypes))
-        return failure();
-      segments[privateClausePos] = privates.size();
-    } else if (keyword == "firstprivate") {
-      // Fail if there was already another firstprivate clause.
-      if (segments[firstprivateClausePos])
-        return allowedOnce(parser, "firstprivate", opName);
-      if (parseOperandAndTypeList(parser, firstprivates, firstprivateTypes))
-        return failure();
-      segments[firstprivateClausePos] = firstprivates.size();
-    } else if (keyword == "shared") {
-      // Fail if there was already another shared clause.
-      if (segments[sharedClausePos])
-        return allowedOnce(parser, "shared", opName);
-      if (parseOperandAndTypeList(parser, shareds, sharedTypes))
-        return failure();
-      segments[sharedClausePos] = shareds.size();
-    } else if (keyword == "copyin") {
-      // Fail if there was already another copyin clause.
-      if (segments[copyinClausePos])
-        return allowedOnce(parser, "copyin", opName);
-      if (parseOperandAndTypeList(parser, copyins, copyinTypes))
-        return failure();
-      segments[copyinClausePos] = copyins.size();
-    } else if (keyword == "allocate") {
-      // Fail if there was already another allocate clause.
-      if (segments[allocateClausePos])
-        return allowedOnce(parser, "allocate", opName);
-      if (parseAllocateAndAllocator(parser, allocates, allocateTypes,
-                                    allocators, allocatorTypes))
-        return failure();
-      segments[allocateClausePos] = allocates.size();
-      segments[allocatorPos] = allocators.size();
-    } else if (keyword == "default") {
-      // Fail if there was already another default clause.
-      if (defaultVal)
-        return allowedOnce(parser, "default", opName);
-      defaultVal = true;
-      StringRef defval;
-      if (parser.parseLParen() || parser.parseKeyword(&defval) ||
-          parser.parseRParen())
-        return failure();
-      // The def prefix is required for the attribute as "private" is a keyword
-      // in C++.
-      auto attr = parser.getBuilder().getStringAttr("def" + defval);
-      result.addAttribute("default_val", attr);
-    } else if (keyword == "proc_bind") {
-      // Fail if there was already another proc_bind clause.
-      if (procBind)
-        return allowedOnce(parser, "proc_bind", opName);
-      procBind = true;
-      StringRef bind;
-      if (parser.parseLParen() || parser.parseKeyword(&bind) ||
-          parser.parseRParen())
-        return failure();
-      auto attr = parser.getBuilder().getStringAttr(bind);
-      result.addAttribute("proc_bind_val", attr);
-    } else {
-      return parser.emitError(parser.getNameLoc())
-             << keyword << " is not a valid clause for the " << opName
-             << " operation";
-    }
-  }
-
-  // Add if parameter.
-  if (segments[ifClausePos] &&
-      parser.resolveOperand(ifCond.first, ifCond.second, result.operands))
-    return failure();
-
-  // Add num_threads parameter.
-  if (segments[numThreadsClausePos] &&
-      parser.resolveOperand(numThreads.first, numThreads.second,
-                            result.operands))
-    return failure();
-
-  // Add private parameters.
-  if (segments[privateClausePos] &&
-      parser.resolveOperands(privates, privateTypes, privates[0].location,
-                             result.operands))
-    return failure();
-
-  // Add firstprivate parameters.
-  if (segments[firstprivateClausePos] &&
-      parser.resolveOperands(firstprivates, firstprivateTypes,
-                             firstprivates[0].location, result.operands))
-    return failure();
-
-  // Add shared parameters.
-  if (segments[sharedClausePos] &&
-      parser.resolveOperands(shareds, sharedTypes, shareds[0].location,
-                             result.operands))
-    return failure();
-
-  // Add copyin parameters.
-  if (segments[copyinClausePos] &&
-      parser.resolveOperands(copyins, copyinTypes, copyins[0].location,
-                             result.operands))
-    return failure();
-
-  // Add allocate parameters.
-  if (segments[allocateClausePos] &&
-      parser.resolveOperands(allocates, allocateTypes, allocates[0].location,
-                             result.operands))
-    return failure();
-
-  // Add allocator parameters.
-  if (segments[allocatorPos] &&
-      parser.resolveOperands(allocators, allocatorTypes, allocators[0].location,
-                             result.operands))
-    return failure();
-
-  result.addAttribute("operand_segment_sizes",
-                      parser.getBuilder().getI32VectorAttr(segments));
-
-  Region *body = result.addRegion();
-  SmallVector<OpAsmParser::OperandType, 4> regionArgs;
-  SmallVector<Type, 4> regionArgTypes;
-  if (parser.parseRegion(*body, regionArgs, regionArgTypes))
-    return failure();
-  return success();
-}
+//===----------------------------------------------------------------------===//
+// Parser and printer for Linear Clause
+//===----------------------------------------------------------------------===//
 
 /// linear ::= `linear` `(` linear-list `)`
 /// linear-list := linear-val | linear-val linear-list
 /// linear-val := ssa-id-and-type `=` ssa-id-and-type
 static ParseResult
 parseLinearClause(OpAsmParser &parser,
                   SmallVectorImpl<OpAsmParser::OperandType> &vars,
                   SmallVectorImpl<Type> &types,
                   SmallVectorImpl<OpAsmParser::OperandType> &stepVars) {
   if (parser.parseLParen())
     return failure();
 
   do {
parseLinearClause(OpAsmParser &parser,
   } while (succeeded(parser.parseOptionalComma()));
 
   if (parser.parseRParen())
     return failure();
 
   return success();
 }
 
+/// Print Linear Clause
+static void printLinearClause(OpAsmPrinter &p, OperandRange linearVars,
+                              OperandRange linearStepVars) {
+  size_t linearVarsSize = linearVars.size();
+  p << "(";
+  for (unsigned i = 0; i < linearVarsSize; ++i) {
+    std::string separator = i == linearVarsSize - 1 ? ") " : ", ";
+    p << linearVars[i];
+    if (linearStepVars.size() > i)
+      p << " = " << linearStepVars[i];
+    p << " : " << linearVars[i].getType() << separator;
+  }
+}
+
+//===----------------------------------------------------------------------===//
+// Parser and printer for Schedule Clause
+//===----------------------------------------------------------------------===//
+
 /// schedule ::= `schedule` `(` sched-list `)`
 /// sched-list ::= sched-val | sched-val sched-list
 /// sched-val ::= sched-with-chunk | sched-wo-chunk
 /// sched-with-chunk ::= sched-with-chunk-types (`=` ssa-id-and-type)?
 /// sched-with-chunk-types ::= `static` | `dynamic` | `guided`
 /// sched-wo-chunk ::=  `auto` | `runtime`
 static ParseResult
 parseScheduleClause(OpAsmParser &parser, SmallString<8> &schedule,
parseScheduleClause(OpAsmParser &parser, SmallString<8> &schedule,
   }
 
   if (parser.parseRParen())
     return failure();
 
   return success();
 }
 
-/// reduction-init ::= `reduction` `(` reduction-entry-list `)`
+/// Print schedule clause
+static void printScheduleClause(OpAsmPrinter &p, StringRef &sched,
+                                Value scheduleChunkVar) {
+  std::string schedLower = sched.lower();
+  p << "(" << schedLower;
+  if (scheduleChunkVar)
+    p << " = " << scheduleChunkVar;
+  p << ") ";
+}
+
+//===----------------------------------------------------------------------===//
+// Parser, printer and verifier for ReductionVarList
+//===----------------------------------------------------------------------===//
+
+/// reduction ::= `reduction` `(` reduction-entry-list `)`
 /// reduction-entry-list ::= reduction-entry
 ///                        | reduction-entry-list `,` reduction-entry
 /// reduction-entry ::= symbol-ref `->` ssa-id `:` type
 static ParseResult
 parseReductionVarList(OpAsmParser &parser,
                       SmallVectorImpl<SymbolRefAttr> &symbols,
                       SmallVectorImpl<OpAsmParser::OperandType> &operands,
                       SmallVectorImpl<Type> &types) {
   if (failed(parser.parseLParen()))
     return failure();
 
   do {
     if (parser.parseAttribute(symbols.emplace_back()) || parser.parseArrow() ||
         parser.parseOperand(operands.emplace_back()) ||
         parser.parseColonType(types.emplace_back()))
       return failure();
   } while (succeeded(parser.parseOptionalComma()));
   return parser.parseRParen();
 }
 
-/// Parses an OpenMP Workshare Loop operation
-///
-/// operation ::= `omp.wsloop` loop-control clause-list
-/// loop-control ::= `(` ssa-id-list `)` `:` type `=`  loop-bounds
-/// loop-bounds := `(` ssa-id-list `)` to `(` ssa-id-list `)` steps
-/// steps := `step` `(`ssa-id-list`)`
-/// clause-list ::= clause | empty | clause-list
-/// clause ::= private | firstprivate | lastprivate | linear | schedule |
-//             collapse | nowait | ordered | order | inclusive
-/// private ::= `private` `(` ssa-id-and-type-list `)`
-/// firstprivate ::= `firstprivate` `(` ssa-id-and-type-list `)`
-/// lastprivate ::= `lastprivate` `(` ssa-id-and-type-list `)`
+/// Print Reduction clause
+static void printReductionVarList(OpAsmPrinter &p,
+                                  Optional<ArrayAttr> reductions,
+                                  OperandRange reduction_vars) {
+  for (unsigned i = 0, e = reductions->size(); i < e; ++i) {
+    if (i != 0)
+      p << ", ";
+    p << (*reductions)[i] << " -> " << reduction_vars[i] << " : "
+      << reduction_vars[i].getType();
+  }
+  p << ") ";
+}
+
+/// Verifies Reduction Clause
+static LogicalResult verifyReductionVarList(Operation *op,
+                                            Optional<ArrayAttr> reductions,
+                                            OperandRange reduction_vars) {
+  if (reduction_vars.size() != 0) {
+    if (!reductions || reductions->size() != reduction_vars.size())
+      return op->emitOpError()
+             << "expected as many reduction symbol references "
+                "as reduction variables";
+  } else {
+    if (reductions)
+      return op->emitOpError() << "unexpected reduction symbol references";
+    return success();
+  }
+
+  DenseSet<Value> accumulators;
+  for (auto args : llvm::zip(reduction_vars, *reductions)) {
+    Value accum = std::get<0>(args);
+
+    if (!accumulators.insert(accum).second)
+      return op->emitOpError() << "accumulator variable used more than once";
+
+    Type varType = accum.getType().cast<PointerLikeType>();
+    auto symbolRef = std::get<1>(args).cast<SymbolRefAttr>();
+    auto decl =
+        SymbolTable::lookupNearestSymbolFrom<ReductionDeclareOp>(op, symbolRef);
+    if (!decl)
+      return op->emitOpError() << "expected symbol reference " << symbolRef
+                               << " to point to a reduction declaration";
+
+    if (decl.getAccumulatorType() && decl.getAccumulatorType() != varType)
+      return op->emitOpError()
+             << "expected accumulator (" << varType
+             << ") to be the same type as reduction declaration ("
+             << decl.getAccumulatorType() << ")";
+  }
+
+  return success();
+}
+
+enum ClauseType {
+  ifClause,
+  numThreadsClause,
+  privateClause,
+  firstprivateClause,
+  lastprivateClause,
+  sharedClause,
+  copyinClause,
+  allocateClause,
+  defaultClause,
+  procBindClause,
+  reductionClause,
+  nowaitClause,
+  linearClause,
+  scheduleClause,
+  collapseClause,
+  orderClause,
+  orderedClause,
+  inclusiveClause,
+  COUNT
+};
+
+//===----------------------------------------------------------------------===//
+// Parser for Clause List
+//===----------------------------------------------------------------------===//
+
+/// 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 | private | firstprivate | lastprivate |
+///            shared | copyin | allocate | default | proc-bind | reduction |
+///            nowait | linear | schedule | collapse | order | ordered |
+///            inclusive
+/// if ::= `if` `(` ssa-id-and-type `)`
+/// num-threads ::= `num_threads` `(` ssa-id-and-type `)`
+/// private ::= `private` operand-and-type-list
+/// firstprivate ::= `firstprivate` operand-and-type-list
+/// lastprivate ::= `lastprivate` operand-and-type-list
+/// shared ::= `shared` operand-and-type-list
+/// copyin ::= `copyin` operand-and-type-list
+/// allocate ::= `allocate` `(` allocate-operand-list `)`
+/// default ::= `default` `(` (`private` | `firstprivate` | `shared` | `none`)
+/// 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 `)`
-/// nowait ::= `nowait`
-/// ordered ::= `ordered` `(` ssa-id-and-type `)`
 /// order ::= `order` `(` `concurrent` `)`
+/// ordered ::= `ordered` `(` ssa-id-and-type `)`
 /// inclusive ::= `inclusive`
 ///
-static ParseResult parseWsLoopOp(OpAsmParser &parser, OperationState &result) {
-  Type loopVarType;
-  int numIVs;
+/// 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) {
 
-  // Parse an opening `(` followed by induction variables followed by `)`
-  SmallVector<OpAsmParser::OperandType> ivs;
-  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
-                                     OpAsmParser::Delimiter::Paren))
-    return failure();
+  // Check done[clause] to see if it has been parsed already
+  llvm::BitVector done(ClauseType::COUNT, false);
 
-  numIVs = static_cast<int>(ivs.size());
+  // See pos[clause] to get position of clause in operand segments
+  SmallVector<int> pos(ClauseType::COUNT, -1);
 
-  if (parser.parseColonType(loopVarType))
-    return failure();
+  // Stores the last parsed clause keyword
+  StringRef clauseKeyword;
+  StringRef opName = result.name.getStringRef();
 
-  // Parse loop bounds.
-  SmallVector<OpAsmParser::OperandType> lower;
-  if (parser.parseEqual() ||
-      parser.parseOperandList(lower, numIVs, OpAsmParser::Delimiter::Paren) ||
-      parser.resolveOperands(lower, loopVarType, result.operands))
-    return failure();
+  // Containers for storing operands, types and attributes for various clauses
+  std::pair<OpAsmParser::OperandType, Type> ifCond;
+  std::pair<OpAsmParser::OperandType, Type> numThreads;
 
-  SmallVector<OpAsmParser::OperandType> upper;
-  if (parser.parseKeyword("to") ||
-      parser.parseOperandList(upper, numIVs, OpAsmParser::Delimiter::Paren) ||
-      parser.resolveOperands(upper, loopVarType, result.operands))
-    return failure();
+  SmallVector<OpAsmParser::OperandType> privates, firstprivates, lastprivates,
+      shareds, copyins;
+  SmallVector<Type> privateTypes, firstprivateTypes, lastprivateTypes,
+      sharedTypes, copyinTypes;
 
-  // Parse step values.
-  SmallVector<OpAsmParser::OperandType> steps;
-  if (parser.parseKeyword("step") ||
-      parser.parseOperandList(steps, numIVs, OpAsmParser::Delimiter::Paren) ||
-      parser.resolveOperands(steps, loopVarType, result.operands))
-    return failure();
+  SmallVector<OpAsmParser::OperandType> allocates, allocators;
+  SmallVector<Type> allocateTypes, allocatorTypes;
 
-  SmallVector<OpAsmParser::OperandType> privates;
-  SmallVector<Type> privateTypes;
-  SmallVector<OpAsmParser::OperandType> firstprivates;
-  SmallVector<Type> firstprivateTypes;
-  SmallVector<OpAsmParser::OperandType> lastprivates;
-  SmallVector<Type> lastprivateTypes;
-  SmallVector<OpAsmParser::OperandType> linears;
-  SmallVector<Type> linearTypes;
-  SmallVector<OpAsmParser::OperandType> linearSteps;
   SmallVector<SymbolRefAttr> reductionSymbols;
   SmallVector<OpAsmParser::OperandType> reductionVars;
   SmallVector<Type> reductionVarTypes;
+
+  SmallVector<OpAsmParser::OperandType> linears;
+  SmallVector<Type> linearTypes;
+  SmallVector<OpAsmParser::OperandType> linearSteps;
+
   SmallString<8> schedule;
   Optional<OpAsmParser::OperandType> scheduleChunkSize;
 
-  const StringRef opName = result.name.getStringRef();
-  StringRef keyword;
-
-  enum SegmentPos {
-    lbPos = 0,
-    ubPos,
-    stepPos,
-    privateClausePos,
-    firstprivateClausePos,
-    lastprivateClausePos,
-    linearClausePos,
-    linearStepPos,
-    reductionVarPos,
-    scheduleClausePos,
+  // 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 == defaultClause || clause == procBindClause ||
+        clause == nowaitClause || clause == collapseClause ||
+        clause == orderClause || clause == orderedClause ||
+        clause == inclusiveClause)
+      continue;
+
+    pos[clause] = currPos++;
+
+    // For the following clauses, two positions are reserved in the operand
+    // segments
+    if (clause == allocateClause || clause == linearClause)
+      currPos++;
+  }
+
+  SmallVector<int> clauseSegments(currPos);
+
+  // Helper function to check if a clause is allowed/repeated or not
+  auto checkAllowed = [&](ClauseType clause,
+                          bool allowRepeat = false) -> ParseResult {
+    if (!llvm::is_contained(clauses, clause))
+      return parser.emitError(parser.getCurrentLocation())
+             << clauseKeyword << "is not a valid clause for the " << opName
+             << " operation";
+    if (done[clause] && !allowRepeat)
+      return parser.emitError(parser.getCurrentLocation())
+             << "at most one " << clauseKeyword << " clause can appear on the "
+             << opName << " operation";
+    done[clause] = true;
+    return success();
   };
-  std::array<int, 10> segments{numIVs, numIVs, numIVs, 0, 0, 0, 0, 0, 0, 0};
 
-  while (succeeded(parser.parseOptionalKeyword(&keyword))) {
-    if (keyword == "private") {
-      if (segments[privateClausePos])
-        return allowedOnce(parser, "private", opName);
-      if (parseOperandAndTypeList(parser, privates, privateTypes))
-        return failure();
-      segments[privateClausePos] = privates.size();
-    } else if (keyword == "firstprivate") {
-      // fail if there was already another firstprivate clause
-      if (segments[firstprivateClausePos])
-        return allowedOnce(parser, "firstprivate", opName);
-      if (parseOperandAndTypeList(parser, firstprivates, firstprivateTypes))
-        return failure();
-      segments[firstprivateClausePos] = firstprivates.size();
-    } else if (keyword == "lastprivate") {
-      // fail if there was already another shared clause
-      if (segments[lastprivateClausePos])
-        return allowedOnce(parser, "lastprivate", opName);
-      if (parseOperandAndTypeList(parser, lastprivates, lastprivateTypes))
-        return failure();
-      segments[lastprivateClausePos] = lastprivates.size();
-    } else if (keyword == "linear") {
-      // fail if there was already another linear clause
-      if (segments[linearClausePos])
-        return allowedOnce(parser, "linear", opName);
-      if (parseLinearClause(parser, linears, linearTypes, linearSteps))
-        return failure();
-      segments[linearClausePos] = linears.size();
-      segments[linearStepPos] = linearSteps.size();
-    } else if (keyword == "schedule") {
-      if (!schedule.empty())
-        return allowedOnce(parser, "schedule", opName);
-      if (parseScheduleClause(parser, schedule, scheduleChunkSize))
+  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 == "private") {
+      if (checkAllowed(privateClause) ||
+          parseOperandAndTypeList(parser, privates, privateTypes))
+        return failure();
+      clauseSegments[pos[privateClause]] = privates.size();
+    } else if (clauseKeyword == "firstprivate") {
+      if (checkAllowed(firstprivateClause) ||
+          parseOperandAndTypeList(parser, firstprivates, firstprivateTypes))
+        return failure();
+      clauseSegments[pos[firstprivateClause]] = firstprivates.size();
+    } else if (clauseKeyword == "lastprivate") {
+      if (checkAllowed(lastprivateClause) ||
+          parseOperandAndTypeList(parser, lastprivates, lastprivateTypes))
+        return failure();
+      clauseSegments[pos[lastprivateClause]] = lastprivates.size();
+    } else if (clauseKeyword == "shared") {
+      if (checkAllowed(sharedClause) ||
+          parseOperandAndTypeList(parser, shareds, sharedTypes))
+        return failure();
+      clauseSegments[pos[sharedClause]] = shareds.size();
+    } else if (clauseKeyword == "copyin") {
+      if (checkAllowed(copyinClause) ||
+          parseOperandAndTypeList(parser, copyins, copyinTypes))
+        return failure();
+      clauseSegments[pos[copyinClause]] = copyins.size();
+    } else if (clauseKeyword == "allocate") {
+      if (checkAllowed(allocateClause) ||
+          parseAllocateAndAllocator(parser, allocates, allocateTypes,
+                                    allocators, allocatorTypes))
+        return failure();
+      clauseSegments[pos[allocateClause]] = allocates.size();
+      clauseSegments[pos[allocateClause] + 1] = allocators.size();
+    } else if (clauseKeyword == "default") {
+      StringRef defval;
+      if (checkAllowed(defaultClause) || parser.parseLParen() ||
+          parser.parseKeyword(&defval) || parser.parseRParen())
+        return failure();
+      // The def prefix is required for the attribute as "private" is a keyword
+      // in C++.
+      auto attr = parser.getBuilder().getStringAttr("def" + defval);
+      result.addAttribute("default_val", attr);
+    } else if (clauseKeyword == "proc_bind") {
+      StringRef bind;
+      if (checkAllowed(procBindClause) || parser.parseLParen() ||
+          parser.parseKeyword(&bind) || parser.parseRParen())
+        return failure();
+      auto attr = parser.getBuilder().getStringAttr(bind);
+      result.addAttribute("proc_bind_val", attr);
+    } else if (clauseKeyword == "reduction") {
+      if (checkAllowed(reductionClause) ||
+          parseReductionVarList(parser, reductionSymbols, reductionVars,
+                                reductionVarTypes))
+        return failure();
+      clauseSegments[pos[reductionClause]] = reductionVars.size();
+    } else if (clauseKeyword == "nowait") {
+      if (checkAllowed(nowaitClause))
+        return failure();
+      auto attr = UnitAttr::get(parser.getBuilder().getContext());
+      result.addAttribute("nowait", attr);
+    } else if (clauseKeyword == "linear") {
+      if (checkAllowed(linearClause) ||
+          parseLinearClause(parser, linears, linearTypes, linearSteps))
+        return failure();
+      clauseSegments[pos[linearClause]] = linears.size();
+      clauseSegments[pos[linearClause] + 1] = linearSteps.size();
+    } else if (clauseKeyword == "schedule") {
+      if (checkAllowed(scheduleClause) ||
+          parseScheduleClause(parser, schedule, scheduleChunkSize))
         return failure();
       if (scheduleChunkSize) {
-        segments[scheduleClausePos] = 1;
+        clauseSegments[pos[scheduleClause]] = 1;
       }
-    } else if (keyword == "collapse") {
+    } else if (clauseKeyword == "collapse") {
       auto type = parser.getBuilder().getI64Type();
       mlir::IntegerAttr attr;
-      if (parser.parseLParen() || parser.parseAttribute(attr, type) ||
-          parser.parseRParen())
+      if (checkAllowed(collapseClause) || parser.parseLParen() ||
+          parser.parseAttribute(attr, type) || parser.parseRParen())
         return failure();
       result.addAttribute("collapse_val", attr);
-    } else if (keyword == "nowait") {
-      auto attr = UnitAttr::get(parser.getContext());
-      result.addAttribute("nowait", attr);
-    } else if (keyword == "ordered") {
+    } else if (clauseKeyword == "ordered") {
       mlir::IntegerAttr attr;
+      if (checkAllowed(orderedClause))
+        return failure();
       if (succeeded(parser.parseOptionalLParen())) {
         auto type = parser.getBuilder().getI64Type();
-        if (parser.parseAttribute(attr, type))
-          return failure();
-        if (parser.parseRParen())
+        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 (keyword == "order") {
+    } else if (clauseKeyword == "order") {
       StringRef order;
-      if (parser.parseLParen() || parser.parseKeyword(&order) ||
-          parser.parseRParen())
+      if (checkAllowed(orderClause) || parser.parseLParen() ||
+          parser.parseKeyword(&order) || parser.parseRParen())
         return failure();
       auto attr = parser.getBuilder().getStringAttr(order);
       result.addAttribute("order", attr);
-    } else if (keyword == "inclusive") {
-      auto attr = UnitAttr::get(parser.getContext());
-      result.addAttribute("inclusive", attr);
-    } else if (keyword == "reduction") {
-      if (segments[reductionVarPos])
-        return allowedOnce(parser, "reduction", opName);
-      if (failed(parseReductionVarList(parser, reductionSymbols, reductionVars,
-                                       reductionVarTypes)))
+    } else if (clauseKeyword == "inclusive") {
+      if (checkAllowed(inclusiveClause))
         return failure();
-      segments[reductionVarPos] = reductionVars.size();
+      auto attr = UnitAttr::get(parser.getBuilder().getContext());
+      result.addAttribute("inclusive", attr);
+    } else {
+      return parser.emitError(parser.getNameLoc())
+             << clauseKeyword << " is not a valid clause";
     }
   }
 
-  if (segments[privateClausePos]) {
-    parser.resolveOperands(privates, privateTypes, privates[0].location,
-                           result.operands);
-  }
+  // Add if parameter.
+  if (done[ifClause] && clauseSegments[pos[ifClause]] &&
+      failed(
+          parser.resolveOperand(ifCond.first, ifCond.second, result.operands)))
+    return failure();
 
-  if (segments[firstprivateClausePos]) {
-    parser.resolveOperands(firstprivates, firstprivateTypes,
-                           firstprivates[0].location, result.operands);
-  }
+  // Add num_threads parameter.
+  if (done[numThreadsClause] && clauseSegments[pos[numThreadsClause]] &&
+      failed(parser.resolveOperand(numThreads.first, numThreads.second,
+                                   result.operands)))
+    return failure();
 
-  if (segments[lastprivateClausePos]) {
-    parser.resolveOperands(lastprivates, lastprivateTypes,
-                           lastprivates[0].location, result.operands);
-  }
+  // Add private parameters.
+  if (done[privateClause] && clauseSegments[pos[privateClause]] &&
+      failed(parser.resolveOperands(privates, privateTypes,
+                                    privates[0].location, result.operands)))
+    return failure();
 
-  if (segments[linearClausePos]) {
-    parser.resolveOperands(linears, linearTypes, linears[0].location,
-                           result.operands);
-    auto linearStepType = parser.getBuilder().getI32Type();
-    SmallVector<Type> linearStepTypes(linearSteps.size(), linearStepType);
-    parser.resolveOperands(linearSteps, linearStepTypes,
-                           linearSteps[0].location, result.operands);
-  }
+  // Add firstprivate parameters.
+  if (done[firstprivateClause] && clauseSegments[pos[firstprivateClause]] &&
+      failed(parser.resolveOperands(firstprivates, firstprivateTypes,
+                                    firstprivates[0].location,
+                                    result.operands)))
+    return failure();
+
+  // Add lastprivate parameters.
+  if (done[lastprivateClause] && clauseSegments[pos[lastprivateClause]] &&
+      failed(parser.resolveOperands(lastprivates, lastprivateTypes,
+                                    lastprivates[0].location, result.operands)))
+    return failure();
+
+  // Add shared parameters.
+  if (done[sharedClause] && clauseSegments[pos[sharedClause]] &&
+      failed(parser.resolveOperands(shareds, sharedTypes, shareds[0].location,
+                                    result.operands)))
+    return failure();
 
-  if (segments[reductionVarPos]) {
+  // Add copyin parameters.
+  if (done[copyinClause] && clauseSegments[pos[copyinClause]] &&
+      failed(parser.resolveOperands(copyins, copyinTypes, copyins[0].location,
+                                    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] &&
+      failed(parser.resolveOperands(allocators, allocatorTypes,
+                                    allocators[0].location, result.operands)))
+    return failure();
+
+  // Add reduction parameters and symbols
+  if (done[reductionClause] && clauseSegments[pos[reductionClause]]) {
     if (failed(parser.resolveOperands(reductionVars, reductionVarTypes,
-                                      parser.getNameLoc(), result.operands))) {
+                                      parser.getNameLoc(), result.operands)))
       return failure();
-    }
+
     SmallVector<Attribute> reductions(reductionSymbols.begin(),
                                       reductionSymbols.end());
     result.addAttribute("reductions",
                         parser.getBuilder().getArrayAttr(reductions));
   }
 
-  if (!schedule.empty()) {
+  // Add linear parameters
+  if (done[linearClause] && clauseSegments[pos[linearClause]]) {
+    auto linearStepType = parser.getBuilder().getI32Type();
+    SmallVector<Type> linearStepTypes(linearSteps.size(), linearStepType);
+    if (failed(parser.resolveOperands(linears, linearTypes, linears[0].location,
+                                      result.operands)) ||
+        failed(parser.resolveOperands(linearSteps, linearStepTypes,
+                                      linearSteps[0].location,
+                                      result.operands)))
+      return failure();
+  }
+
+  // Add schedule parameters
+  if (done[scheduleClause] && !schedule.empty()) {
     schedule[0] = llvm::toUpper(schedule[0]);
     auto attr = parser.getBuilder().getStringAttr(schedule);
     result.addAttribute("schedule_val", attr);
     if (scheduleChunkSize) {
       auto chunkSizeType = parser.getBuilder().getI32Type();
       parser.resolveOperand(*scheduleChunkSize, chunkSizeType, result.operands);
     }
   }
 
+  segments.insert(segments.end(), clauseSegments.begin(), clauseSegments.end());
+
+  return success();
+}
+
+/// Parses a parallel operation.
+///
+/// operation ::= `omp.parallel` clause-list
+/// clause-list ::= clause | clause clause-list
+/// clause ::= if | num-threads | private | firstprivate | shared | copyin |
+///            allocate | default | proc-bind
+///
+static ParseResult parseParallelOp(OpAsmParser &parser,
+                                   OperationState &result) {
+  SmallVector<ClauseType> clauses = {
+      ifClause,           numThreadsClause, privateClause,
+      firstprivateClause, sharedClause,     copyinClause,
+      allocateClause,     defaultClause,    procBindClause};
+
+  SmallVector<int> segments;
+
+  if (failed(parseClauses(parser, result, clauses, segments)))
+    return failure();
+
+  result.addAttribute("operand_segment_sizes",
+                      parser.getBuilder().getI32VectorAttr(segments));
+
+  Region *body = result.addRegion();
+  SmallVector<OpAsmParser::OperandType> regionArgs;
+  SmallVector<Type> regionArgTypes;
+  if (parser.parseRegion(*body, regionArgs, regionArgTypes))
+    return failure();
+  return success();
+}
+
+/// Parses an OpenMP Workshare Loop operation
+///
+/// wsloop ::= `omp.wsloop` loop-control clause-list
+/// loop-control ::= `(` ssa-id-list `)` `:` type `=`  loop-bounds
+/// loop-bounds := `(` ssa-id-list `)` to `(` ssa-id-list `)` steps
+/// steps := `step` `(`ssa-id-list`)`
+/// clause-list ::= clause clause-list | empty
+/// clause ::= private | firstprivate | lastprivate | linear | schedule |
+//             collapse | nowait | ordered | order | inclusive | reduction
+static ParseResult parseWsLoopOp(OpAsmParser &parser, OperationState &result) {
+
+  // Parse an opening `(` followed by induction variables followed by `)`
+  SmallVector<OpAsmParser::OperandType> ivs;
+  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
+                                     OpAsmParser::Delimiter::Paren))
+    return failure();
+
+  int numIVs = static_cast<int>(ivs.size());
+  Type loopVarType;
+  if (parser.parseColonType(loopVarType))
+    return failure();
+
+  // Parse loop bounds.
+  SmallVector<OpAsmParser::OperandType> lower;
+  if (parser.parseEqual() ||
+      parser.parseOperandList(lower, numIVs, OpAsmParser::Delimiter::Paren) ||
+      parser.resolveOperands(lower, loopVarType, result.operands))
+    return failure();
+
+  SmallVector<OpAsmParser::OperandType> upper;
+  if (parser.parseKeyword("to") ||
+      parser.parseOperandList(upper, numIVs, OpAsmParser::Delimiter::Paren) ||
+      parser.resolveOperands(upper, loopVarType, result.operands))
+    return failure();
+
+  // Parse step values.
+  SmallVector<OpAsmParser::OperandType> steps;
+  if (parser.parseKeyword("step") ||
+      parser.parseOperandList(steps, numIVs, OpAsmParser::Delimiter::Paren) ||
+      parser.resolveOperands(steps, loopVarType, result.operands))
+    return failure();
+
+  SmallVector<ClauseType> clauses = {
+      privateClause,   firstprivateClause, lastprivateClause, linearClause,
+      reductionClause, collapseClause,     orderClause,       orderedClause,
+      nowaitClause,    scheduleClause};
+  SmallVector<int> segments{numIVs, numIVs, numIVs};
+  if (failed(parseClauses(parser, result, clauses, segments)))
+    return failure();
+
   result.addAttribute("operand_segment_sizes",
                       parser.getBuilder().getI32VectorAttr(segments));
 
   // Now parse the body.
   Region *body = result.addRegion();
   SmallVector<Type> ivTypes(numIVs, loopVarType);
   SmallVector<OpAsmParser::OperandType> blockArgs(ivs);
   if (parser.parseRegion(*body, blockArgs, ivTypes))
     return failure();
   return success();
 }
 
 static void printWsLoopOp(OpAsmPrinter &p, WsLoopOp op) {
   auto args = op.getRegion().front().getArguments();
   p << " (" << args << ") : " << args[0].getType() << " = (" << op.lowerBound()
-    << ") to (" << op.upperBound() << ") step (" << op.step() << ")";
+    << ") to (" << op.upperBound() << ") step (" << op.step() << ") ";
 
-  // Print private, firstprivate, shared and copyin parameters
-  auto printDataVars = [&p](StringRef name, OperandRange vars) {
-    if (vars.empty())
-      return;
-
-    p << " " << name << "(";
-    llvm::interleaveComma(
-        vars, p, [&](const Value &v) { p << v << " : " << v.getType(); });
-    p << ")";
-  };
-  printDataVars("private", op.private_vars());
-  printDataVars("firstprivate", op.firstprivate_vars());
-  printDataVars("lastprivate", op.lastprivate_vars());
-
-  auto linearVars = op.linear_vars();
-  auto linearVarsSize = linearVars.size();
-  if (linearVarsSize) {
-    p << " "
-      << "linear"
-      << "(";
-    for (unsigned i = 0; i < linearVarsSize; ++i) {
-      std::string separator = i == linearVarsSize - 1 ? ")" : ", ";
-      p << linearVars[i];
-      if (op.linear_step_vars().size() > i)
-        p << " = " << op.linear_step_vars()[i];
-      p << " : " << linearVars[i].getType() << separator;
-    }
+  printDataVars(p, op.private_vars(), "private");
+  printDataVars(p, op.firstprivate_vars(), "firstprivate");
+  printDataVars(p, op.lastprivate_vars(), "lastprivate");
+
+  if (op.linear_vars().size()) {
+    p << "linear";
+    printLinearClause(p, op.linear_vars(), op.linear_step_vars());
   }
 
   if (auto sched = op.schedule_val()) {
-    auto schedLower = sched->lower();
-    p << " schedule(" << schedLower;
-    if (auto chunk = op.schedule_chunk_var()) {
-      p << " = " << chunk;
-    }
-    p << ")";
+    p << "schedule";
+    printScheduleClause(p, sched.getValue(), op.schedule_chunk_var());
   }
 
   if (auto collapse = op.collapse_val())
-    p << " collapse(" << collapse << ")";
+    p << "collapse(" << collapse << ") ";
 
   if (op.nowait())
-    p << " nowait";
+    p << "nowait ";
 
-  if (auto ordered = op.ordered_val()) {
-    p << " ordered(" << ordered << ")";
-  }
+  if (auto ordered = op.ordered_val())
+    p << "ordered(" << ordered << ") ";
 
   if (!op.reduction_vars().empty()) {
-    p << " reduction(";
-    for (unsigned i = 0, e = op.getNumReductionVars(); i < e; ++i) {
-      if (i != 0)
-        p << ", ";
-      p << (*op.reductions())[i] << " -> " << op.reduction_vars()[i] << " : "
-        << op.reduction_vars()[i].getType();
-    }
-    p << ")";
+    p << "reduction(";
+    printReductionVarList(p, op.reductions(), op.reduction_vars());
   }
 
   if (op.inclusive()) {
-    p << " inclusive";
+    p << "inclusive ";
   }
 
   p.printRegion(op.region(), /*printEntryBlockArgs=*/false);
 }
 
 //===----------------------------------------------------------------------===//
 // ReductionOp
 //===----------------------------------------------------------------------===//
if (buildBody) {
     OpBuilder::InsertionGuard guard(builder);
     unsigned numIVs = steps.size();
     SmallVector<Type, 8> argTypes(numIVs, steps.getType().front());
     builder.createBlock(bodyRegion, {}, argTypes);
   }
 }
 
 static LogicalResult verifyWsLoopOp(WsLoopOp op) {
-  if (op.getNumReductionVars() != 0) {
-    if (!op.reductions() ||
-        op.reductions()->size() != op.getNumReductionVars()) {
-      return op.emitOpError() << "expected as many reduction symbol references "
-                                 "as reduction variables";
-    }
-  } else {
-    if (op.reductions())
-      return op.emitOpError() << "unexpected reduction symbol references";
-    return success();
-  }
-
-  DenseSet<Value> accumulators;
-  for (auto args : llvm::zip(op.reduction_vars(), *op.reductions())) {
-    Value accum = std::get<0>(args);
-    if (!accumulators.insert(accum).second) {
-      return op.emitOpError() << "accumulator variable used more than once";
-    }
-    Type varType = accum.getType().cast<PointerLikeType>();
-    auto symbolRef = std::get<1>(args).cast<SymbolRefAttr>();
-    auto decl =
-        SymbolTable::lookupNearestSymbolFrom<ReductionDeclareOp>(op, symbolRef);
-    if (!decl) {
-      return op.emitOpError() << "expected symbol reference " << symbolRef
-                              << " to point to a reduction declaration";
-    }
-
-    if (decl.getAccumulatorType() && decl.getAccumulatorType() != varType) {
-      return op.emitOpError()
-             << "expected accumulator (" << varType
-             << ") to be the same type as reduction declaration ("
-             << decl.getAccumulatorType() << ")";
-    }
-  }
-
-  return success();
+  return verifyReductionVarList(op, op.reductions(), op.reduction_vars());
 }
 
 //===----------------------------------------------------------------------===//
 // 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`.

mlir/test/Dialect/OpenMP/invalid.mlir

 // RUN: mlir-opt -split-input-file -verify-diagnostics %s
 
 func @unknown_clause() {
-  // expected-error@+1 {{invalid is not a valid clause for the omp.parallel operation}}
+  // expected-error@+1 {{invalid is not a valid clause}}
   omp.parallel invalid {
   }
 
   return
 }
 
 // -----