[mlir][OpenMP]Improve OpenMP schedule modifier error checking and tests

[Leporacanthicus]

Summary

Add more thorough error checking in MLIR for OpenMP worksharing loops.

In particular:

1. Only allow 1-2 modifiers.
2. Do not allow "wrong order" of modifiers.
3. Insert dummy "none" for monotonic/nonmonotonic modifier when only SIMD is present.

Add tests to cover failure cases, as well as simd being used on its
own in positive cases.

[Leporacanthicus]

As this is now squashed into D111051, this review is no longer required.

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

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;
   }
 }
 
+static ParseResult
+validateModifiers(OpAsmParser &parser,
+                  SmallVectorImpl<SmallString<12>> &modifiers) {
+  if (modifiers.size() > 2)
+    return parser.emitError(parser.getNameLoc()) << " unexpected modifier(s)";
+  // If we have one modifier that is "simd", then stick a "none" modiifer in
+  // index 0.
+  if (modifiers.size() == 1) {
+    if (modifiers[0] == "simd") {
+      modifiers.push_back(modifiers[0]);
+      modifiers[0] = "none";
+    }
+  } else if (modifiers.size() == 2 &&
+             (modifiers[0] == "simd" ||
+              (modifiers[1] != "simd" && modifiers[1] != "none")))
+    return parser.emitError(parser.getNameLoc()) << " incorrect modifier order";
+  return success();
+}
+
 //===----------------------------------------------------------------------===//
 // 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-list ::= sched-val | sched-val sched-list | sched-val `,`
+/// sched-modifier 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`
+/// sched-modifier ::=  sched-modifier-val | sched-modifier-val `,`
+/// sched-modifier-val sched-modifier ::=  `monotonic` | `nonmonotonic` | `simd`
+/// | `none`
 static ParseResult
 parseScheduleClause(OpAsmParser &parser, SmallString<8> &schedule,
                     SmallVectorImpl<SmallString<12>> &modifiers,
                     Optional<OpAsmParser::OperandType> &chunkSize) {
   if (parser.parseLParen())
     return failure();
 
   StringRef keyword;
if (keyword == "static" || keyword == "dynamic" || keyword == "guided") {
     return parser.emitError(parser.getNameLoc()) << " expected schedule kind";
   }
 
   // If there is a comma, we have one or more modifiers..
   while (succeeded(parser.parseOptionalComma())) {
     StringRef mod;
     if (parser.parseKeyword(&mod))
       return failure();
+    if (mod != "none" && mod != "monotonic" && mod != "nonmonotonic" &&
+        mod != "simd")
+      return parser.emitError(parser.getNameLoc())
+             << " unknown modifier type: " << mod;
     modifiers.push_back(mod);
   }
 
+  if (validateModifiers(parser, modifiers))
+    return failure();
+
   if (parser.parseRParen())
     return failure();
 
   return success();
 }
 
 /// Print schedule clause
 static void printScheduleClause(OpAsmPrinter &p, StringRef &sched,

mlir/test/Target/LLVMIR/openmp-llvm-bad-schedule-modifier.mlir

+// RUN: not mlir-translate -mlir-to-llvmir -split-input-file %s 2>&1 | FileCheck %s
+
+llvm.func @test_omp_wsloop_dynamic_bad_modifier(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, ginandtonic) {
+  // CHECK: unknown modifier type: ginandtonic
+   omp.yield
+ }
+ llvm.return
+}
+
+// -----
+
+llvm.func @test_omp_wsloop_dynamic_many_modifier(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, monotonic, monotonic, monotonic) {
+  // CHECK: unexpected modifier(s)
+   omp.yield
+ }
+ llvm.return
+}
+
+// -----
+
+llvm.func @test_omp_wsloop_dynamic_wrong_modifier(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, simd, monotonic) {
+   // CHECK: incorrect modifier order
+   omp.yield
+ }
+ llvm.return
+}
+
+// -----
+
+llvm.func @test_omp_wsloop_dynamic_wrong_modifier2(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, monotonic, monotonic) {
+   // CHECK: incorrect modifier order
+   omp.yield
+ }
+ llvm.return
+}
+
+// -----
+
+llvm.func @test_omp_wsloop_dynamic_wrong_modifier3(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, simd, simd) {
+   // CHECK: incorrect modifier order
+   omp.yield
+ }
+ llvm.return
+}

mlir/test/Target/LLVMIR/openmp-llvm.mlir

omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, monotonic) {
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
   // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
    llvm.call @body(%iv) : (i64) -> ()
    omp.yield
  }
  llvm.return
 }
 
+llvm.func @test_omp_wsloop_runtime_simd(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(runtime, simd) {
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 47
+  // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
+  // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
+  // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
+   llvm.call @body(%iv) : (i64) -> ()
+   omp.yield
+ }
+ llvm.return
+}
+
+llvm.func @test_omp_wsloop_guided_simd(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(guided, simd) {
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 46
+  // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
+  // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
+  // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
+   llvm.call @body(%iv) : (i64) -> ()
+   omp.yield
+ }
+ llvm.return
+}
+
 // -----
 
 omp.critical.declare @mutex hint(contended)
 
 // CHECK-LABEL: @omp_critical
 llvm.func @omp_critical(%x : !llvm.ptr<i32>, %xval : i32) -> () {
   // CHECK: call void @__kmpc_critical({{.*}}critical_user_.var{{.*}})
   // CHECK: br label %omp.critical.region