[Mlir-commits] [mlir] 2e69944 - [mlir][openacc][NFC] Fix current gang clause parser

[Valentin Clement]

Author: Valentin Clement
Date: 2023-06-13T11:16:58-07:00
New Revision: 2e69944a612e376f8035f0f9e1eb8cca5dbe3fcb

URL: https://github.com/llvm/llvm-project/commit/2e69944a612e376f8035f0f9e1eb8cca5dbe3fcb
DIFF: https://github.com/llvm/llvm-project/commit/2e69944a612e376f8035f0f9e1eb8cca5dbe3fcb.diff

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

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.

Reviewed By: razvanlupusoru, jeanPerier

Differential Revision: https://reviews.llvm.org/D151970

Added: 
    

Modified: 
    mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
    mlir/test/Dialect/OpenACC/invalid.mlir
    mlir/test/Dialect/OpenACC/ops.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index b2998b736f991..d722d5aef2db2 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -634,6 +634,22 @@ void acc::HostDataOp::getCanonicalizationPatterns(RewritePatternSet &results,
 // 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,
@@ -641,31 +657,46 @@ parseGangClause(OpAsmParser &parser,
                 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())
-        return failure();
-      gangStatic = OpAsmParser::UnresolvedOperand{};
-      if (parser.parseOperand(*gangStatic) ||
-          parser.parseColonType(gangStaticType))
+
+      if (!newValue && needValue) {
+        parser.emitError(parser.getCurrentLocation(),
+                         "new value expected after comma");
         return failure();
+      }
+
+      if (!newValue)
+        break;
     }
-    // FIXME: Why allow optional last commas?
-    (void)parser.parseOptionalComma();
+
+    if (!gangNum && !gangStatic) {
+      parser.emitError(parser.getCurrentLocation(),
+                       "expect num and/or static value(s)");
+      return failure();
+    }
+
     if (failed(parser.parseRParen()))
       return failure();
   }

diff  --git a/mlir/test/Dialect/OpenACC/invalid.mlir b/mlir/test/Dialect/OpenACC/invalid.mlir
index 3c1ac1dfbfd21..ebd814205d230 100644
--- a/mlir/test/Dialect/OpenACC/invalid.mlir
+++ b/mlir/test/Dialect/OpenACC/invalid.mlir
@@ -395,6 +395,14 @@ acc.firstprivate.recipe @privatization_i32 : i32 init {
 
 // -----
 
+// expected-error at +1 {{expected ')'}}
+acc.loop gang(static=%i64Value: i64, num=%i64Value: i64 {
+  "test.openacc_dummy_op"() : () -> ()
+  acc.yield
+}
+
+// -----
+
 // expected-error at +1 {{expects non-empty init region}}
 acc.reduction.recipe @reduction_i64 : i64 reduction_operator<add> init {
 } combiner {}
@@ -465,9 +473,25 @@ acc.reduction.recipe @reduction_i64 : i64 reduction_operator<add> init {
 
 // -----
 
+// expected-error at +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 at +1 {{expected symbol reference @privatization_i32 to point to a private declaration}}
   acc.serial private(@privatization_i32 -> %0 : !llvm.ptr<i32>) {
   }
   return
 }
+
+// -----
+
+// expected-error at +1 {{expect num and/or static value(s)}}
+acc.loop gang() {
+  "test.openacc_dummy_op"() : () -> ()
+  acc.yield
+}

diff  --git a/mlir/test/Dialect/OpenACC/ops.mlir b/mlir/test/Dialect/OpenACC/ops.mlir
index 17a6cbdfdc9ee..95553ee3885b1 100644
--- a/mlir/test/Dialect/OpenACC/ops.mlir
+++ b/mlir/test/Dialect/OpenACC/ops.mlir
@@ -272,6 +272,10 @@ func.func @testloopop() -> () {
     "test.openacc_dummy_op"() : () -> ()
     acc.yield
   }
+  acc.loop gang(static=%i64Value: i64, num=%i64Value: i64) {
+    "test.openacc_dummy_op"() : () -> ()
+    acc.yield
+  }
   return
 }
 
@@ -334,6 +338,10 @@ func.func @testloopop() -> () {
 // 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: }
 
 // -----