Page MenuHomePhabricator

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

Authored by kiranktp on Sep 27 2020, 4:58 AM.

Details

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.

Diff Detail

Event Timeline

kiranktp created this revision.Sep 27 2020, 4:58 AM
kiranktp requested review of this revision.Sep 27 2020, 4:58 AM
mehdi_amini added inline comments.Sep 27 2020, 9:44 AM
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 added inline comments.Sep 27 2020, 6:32 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
127

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

kiranktp added inline comments.Sep 27 2020, 10:46 PM
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 updated this revision to Diff 294604.Sep 27 2020, 10:48 PM

Incorporated review comments.

kiranktp marked 2 inline comments as done.Sep 27 2020, 10:49 PM
This revision is now accepted and ready to land.Sep 28 2020, 11:56 AM