[mlir][openacc][NFC] Fix current gang clause parser

[clementval]

Summary

The custom parser for the gang values was not implemented correctly.
This patch fixes the noted issue and allows the num/static values
to appear in any order.

[jeanPerier]

LGTM

[razvanlupusoru]

Thank you!

[clementval]

Rebase

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp

void acc::HostDataOp::getCanonicalizationPatterns(RewritePatternSet &results,
   results.add<RemoveConstantIfConditionWithRegion<HostDataOp>>(context);
 }
 
 //===----------------------------------------------------------------------===//
 // LoopOp
 //===----------------------------------------------------------------------===//
 
 static ParseResult
+parseGangValue(OpAsmParser &parser, llvm::StringRef keyword,
+               std::optional<OpAsmParser::UnresolvedOperand> &value,
+               Type &valueType, bool &needComa, bool &newValue) {
+  if (succeeded(parser.parseOptionalKeyword(keyword))) {
+    if (parser.parseEqual())
+      return failure();
+    value = OpAsmParser::UnresolvedOperand{};
+    if (parser.parseOperand(*value) || parser.parseColonType(valueType))
+      return failure();
+    needComa = true;
+    newValue = true;
+  }
+  return success();
+}
+
+static ParseResult
 parseGangClause(OpAsmParser &parser,
                 std::optional<OpAsmParser::UnresolvedOperand> &gangNum,
                 Type &gangNumType,
                 std::optional<OpAsmParser::UnresolvedOperand> &gangStatic,
                 Type &gangStaticType, UnitAttr &hasGang) {
   hasGang = UnitAttr::get(parser.getBuilder().getContext());
+  gangNum = std::nullopt;
+  gangStatic = std::nullopt;
+  bool needComa = false;
+
   // optional gang operands
   if (succeeded(parser.parseOptionalLParen())) {
-    if (succeeded(parser.parseOptionalKeyword(LoopOp::getGangNumKeyword()))) {
-      if (parser.parseEqual())
+    while (true) {
+      bool newValue = false;
+      bool needValue = false;
+      if (needComa) {
+        if (succeeded(parser.parseOptionalComma()))
+          needValue = true; // expect a new value after comma.
+        else
+          break;
+      }
+
+      if (failed(parseGangValue(parser, LoopOp::getGangNumKeyword(), gangNum,
+                                gangNumType, needComa, newValue)))
         return failure();
-      gangNum = OpAsmParser::UnresolvedOperand{};
-      if (parser.parseOperand(*gangNum) || parser.parseColonType(gangNumType))
+      if (failed(parseGangValue(parser, LoopOp::getGangStaticKeyword(),
+                                gangStatic, gangStaticType, needComa,
+                                newValue)))
         return failure();
-    } else {
-      gangNum = std::nullopt;
-    }
-    // FIXME: Comma should require subsequent operands.
-    (void)parser.parseOptionalComma();
-    if (succeeded(
-            parser.parseOptionalKeyword(LoopOp::getGangStaticKeyword()))) {
-      gangStatic = OpAsmParser::UnresolvedOperand{};
-      if (parser.parseEqual())
+
+      if (!newValue && needValue) {
+        parser.emitError(parser.getCurrentLocation(),
+                         "new value expected after comma");
         return failure();
-      gangStatic = OpAsmParser::UnresolvedOperand{};
-      if (parser.parseOperand(*gangStatic) ||
-          parser.parseColonType(gangStaticType))
-        return failure();
+      }
+
+      if (!newValue)
+        break;
+    }
+
+    if (!gangNum && !gangStatic) {
+      parser.emitError(parser.getCurrentLocation(),
+                       "expect num and/or static value(s)");
     }
-    // FIXME: Why allow optional last commas?
-    (void)parser.parseOptionalComma();
+
     if (failed(parser.parseRParen()))
       return failure();
   }
   return success();
 }
 
 void printGangClause(OpAsmPrinter &p, Operation *op, Value gangNum,
                      Type gangNumType, Value gangStatic, Type gangStaticType,

mlir/test/Dialect/OpenACC/invalid.mlir

^bb0(%arg0 : i32, %arg1 : !llvm.ptr<i32>):
   acc.yield
 } destroy {
 ^bb0(%arg0 : f32):
   acc.yield
 }
 
 // -----
 
+// expected-error@+1 {{expected ')'}}
+acc.loop gang(static=%i64Value: i64, num=%i64Value: i64 {
+  "test.openacc_dummy_op"() : () -> ()
+  acc.yield
+}
+
+// -----
+
 // expected-error@+1 {{expects non-empty init region}}
 acc.reduction.recipe @reduction_i64 : i64 reduction_operator<add> init {
 } combiner {}
 
 // -----
 
 // expected-error@+1 {{expects init region with one argument of the reduction type}}
 acc.reduction.recipe @reduction_i64 : i64 reduction_operator<add> init {

 } combiner {
 ^bb0(%0: i64, %1: i64):
   %2 = arith.constant 0 : i32
   acc.yield %2 : i32
 }
 
 // -----
 
+// expected-error@+1 {{new value expected after comma}}
+acc.loop gang(static=%i64Value: i64, ) {
+  "test.openacc_dummy_op"() : () -> ()
+  acc.yield
+}
+
+// -----
+
 func.func @fct1(%0 : !llvm.ptr<i32>) -> () {
   // expected-error@+1 {{expected symbol reference @privatization_i32 to point to a private declaration}}
   acc.serial private(@privatization_i32 -> %0 : !llvm.ptr<i32>) {
   }
   return
 }
+
+// -----
+
+// expected-error@+1 {{expect num and/or static value(s)}}
+acc.loop gang() {
+  "test.openacc_dummy_op"() : () -> ()
+  acc.yield
+}

mlir/test/Dialect/OpenACC/ops.mlir

func.func @testloopop() -> () {
   acc.loop tile(%i64Value, %i64Value : i64, i64) {
     "test.openacc_dummy_op"() : () -> ()
     acc.yield
   }
   acc.loop tile(%i32Value, %i32Value : i32, i32) {
     "test.openacc_dummy_op"() : () -> ()
     acc.yield
   }
+  acc.loop gang(static=%i64Value: i64, num=%i64Value: i64) {
+    "test.openacc_dummy_op"() : () -> ()
+    acc.yield
+  }
   return
 }
 
 // CHECK:      [[I64VALUE:%.*]] = arith.constant 1 : i64
 // CHECK-NEXT: [[I32VALUE:%.*]] = arith.constant 128 : i32
 // CHECK-NEXT: [[IDXVALUE:%.*]] = arith.constant 8 : index
 // CHECK:      acc.loop gang worker vector {
 // CHECK-NEXT:   "test.openacc_dummy_op"() : () -> ()

 // CHECK:      acc.loop tile([[I64VALUE]], [[I64VALUE]] : i64, i64) {
 // CHECK-NEXT:   "test.openacc_dummy_op"() : () -> ()
 // CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK:      acc.loop tile([[I32VALUE]], [[I32VALUE]] : i32, i32) {
 // CHECK-NEXT:   "test.openacc_dummy_op"() : () -> ()
 // CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
+// CHECK:      acc.loop gang(num=[[I64VALUE]] : i64, static=[[I64VALUE]] : i64) {
+// CHECK-NEXT:   "test.openacc_dummy_op"() : () -> ()
+// CHECK-NEXT:   acc.yield
+// CHECK-NEXT: }
 
 // -----
 
 func.func @acc_loop_multiple_block() {
   acc.parallel {
     acc.loop {
       %c1 = arith.constant 1 : index
       cf.br ^bb1(%c1 : index)