[MLIR][OpenMP] Removed the ambiguity in flush op assembly syntax

[kiranktp]

Summary

Bugzilla Ticket No: Bug 46884 [https://bugs.llvm.org/show_bug.cgi?id=46884]

Flush op assembly syntax was ambiguous:

Consider the below test case:

flush operation is not having any arguments.
But the next statement token i.e "%2" is read as the argument for flush operation and then translator issues an error.

---

$ cat -n flush.mlir

1  llvm.func @_QQmain(%arg0: !llvm.i32) {
2    %0 = llvm.mlir.constant(1 : i64) : !llvm.i64
3    %1 = llvm.alloca %0 x !llvm.i32 {in_type = i32, name = "a"} : (!llvm.i64) -> !llvm.ptr<i32>
4    omp.flush
5    %2 = llvm.load %1 : !llvm.ptr<i32>
6    llvm.return
7  }

$ mlir-translate -mlir-to-llvmir flush.mlir
flush.mlir:5:6: error: expected ':'

%2 = llvm.load %1 : !llvm.ptr<i32>
   ^

---

Solution:

Introduced begin ( ( ) and end token ( ) ) to determince the begin and end of variadic arguments.

The patch includes code changes and testcase modifications.

[mehdi_amini]

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

127

Is this intended to put the attr-dict inside the parenthesis? Usually they are mostly trailing after the operands.

Also you could keep the parentheses optional as well by adding them inside the optional group, what do you think of:

( `(` $varList^ `:` type($varList) `)` )? attr-dict

[clementval]

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

127

+1 for the optional parentheses. attr-dict position looks weird ... is it intended?

[kiranktp]

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

127

Thanks for reviewing Mehdi!, Valentine!

Also you could keep the parentheses optional as well
+1 for the optional parentheses.

Thanks for the suggestion. I will make these parenthesis optional too.

attr-dict position looks weird ... is it intended?

In all the examples that i have seen in documentation, attr-dict was in the beginning. So I didn't try to change the position as it was not causing any problem.
I will move it to the end.

[kiranktp]

Incorporated review comments.

[clementval]

LGTM

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

let description = [{
     The flush construct executes the OpenMP flush operation. This operation
     makes a thread’s temporary view of memory consistent with memory and
     enforces an order on the memory operations of the variables explicitly
     specified or implied.
   }];
 
   let arguments = (ins Variadic<AnyType>:$varList);
 
-  let assemblyFormat = "attr-dict ($varList^ `:` type($varList))?";
+  let assemblyFormat = [{ `(` attr-dict ($varList^ `:` type($varList))? `)` }];
 }
 
 //===----------------------------------------------------------------------===//
 // 2.16 master Construct
 //===----------------------------------------------------------------------===//
 def MasterOp : OpenMP_Op<"master"> {
   let summary = "master construct";
   let description = [{

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

^bb1(%0: index, %1: index):
     // CHECK-NEXT: llvm.cond_br %[[CMP]], ^[[BB2:.*]](%{{[0-9]+}}, %{{[0-9]+}} : !llvm.i64, !llvm.i64), ^[[BB3:.*]]
     cond_br %2, ^bb2(%end, %end : index, index), ^bb3
   // CHECK-NEXT: ^[[BB2]](%[[ARG3:[0-9]+]]: !llvm.i64, %[[ARG4:[0-9]+]]: !llvm.i64):
   ^bb2(%3: index, %4: index):
     // CHECK-NEXT: llvm.br ^[[BB1]](%[[ARG3]], %[[ARG4]] : !llvm.i64, !llvm.i64)
     br ^bb1(%3, %4 : index, index)
   // CHECK-NEXT: ^[[BB3]]:
   ^bb3:
-    omp.flush
+    omp.flush()
     omp.barrier
     omp.taskwait
     omp.taskyield
     omp.terminator
   }
   return
 }

mlir/test/Dialect/OpenMP/ops.mlir

 
 func @omp_taskyield() -> () {
   // CHECK: omp.taskyield
   omp.taskyield
   return
 }
 
 // CHECK-LABEL: func @omp_flush
-// CHECK-SAME: %[[ARG0:.*]]: !llvm.i32
+// CHECK-SAME: ([[ARG0:%.*]]: !llvm.i32) {
 func @omp_flush(%arg0 : !llvm.i32) -> () {
   // Test without data var
-  // CHECK: omp.flush
-  omp.flush
+  // CHECK: omp.flush()
+  omp.flush ()
 
   // Test with one data var
-  // CHECK: omp.flush %[[ARG0]] : !llvm.i32
-  "omp.flush"(%arg0) : (!llvm.i32) -> ()
+  // CHECK: omp.flush( [[ARG0]] : !llvm.i32)
+  omp.flush(%arg0 : !llvm.i32)
 
   // Test with two data var
-  // CHECK: omp.flush %[[ARG0]], %[[ARG0]] : !llvm.i32, !llvm.i32
-  "omp.flush"(%arg0, %arg0): (!llvm.i32, !llvm.i32) -> ()
+  // CHECK: omp.flush( [[ARG0]], [[ARG0]] : !llvm.i32, !llvm.i32)
+  omp.flush(%arg0, %arg0: !llvm.i32, !llvm.i32)
 
   return
 }
 
 func @omp_terminator() -> () {
   // CHECK: omp.terminator
   omp.terminator
 }

mlir/test/Target/openmp-llvm.mlir

llvm.func @test_stand_alone_directives() {
 
   // CHECK-NEXT:    ret void
   llvm.return
 }
 
 // CHECK-LABEL: define void @test_flush_construct(i32 %0)
 llvm.func @test_flush_construct(%arg0: !llvm.i32) {
   // CHECK: call void @__kmpc_flush(%struct.ident_t* @{{[0-9]+}}
-  omp.flush
+  omp.flush ()
 
   // CHECK: call void @__kmpc_flush(%struct.ident_t* @{{[0-9]+}}
-  omp.flush %arg0 : !llvm.i32
+  omp.flush (%arg0 : !llvm.i32)
 
   // CHECK: call void @__kmpc_flush(%struct.ident_t* @{{[0-9]+}}
-  omp.flush %arg0, %arg0 : !llvm.i32, !llvm.i32
+  omp.flush (%arg0, %arg0 : !llvm.i32, !llvm.i32)
+
+  %0 = llvm.mlir.constant(1 : i64) : !llvm.i64
+  //  CHECK: alloca {{.*}} align 4
+  %1 = llvm.alloca %0 x !llvm.i32 {in_type = i32, name = "a"} : (!llvm.i64) -> !llvm.ptr<i32>
+  // CHECK: call void @__kmpc_flush(%struct.ident_t* @{{[0-9]+}}
+  omp.flush ()
+  //  CHECK: load i32, i32*
+  %2 = llvm.load %1 : !llvm.ptr<i32>
 
   // CHECK-NEXT:    ret void
   llvm.return
 }
 
 // CHECK-LABEL: define void @test_omp_parallel_1()
 llvm.func @test_omp_parallel_1() -> () {
   // CHECK: call void{{.*}}@__kmpc_fork_call{{.*}}@[[OMP_OUTLINED_FN_1:.*]] to {{.*}}