[PATCH] D79410: [MLIR] [OpenMP] Add basic OpenMP parallel operation

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Can you add a pointers to

1. the discourse discussion for context in the summary?

https://llvm.discourse.group/t/openmp-parallel-operation-design-issues/686

2. the openmp spec ( I think the link above has a pointer to it). Also say allocate from 5.0 is not there.

Some comments inline.



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:102
+    Arguments<(ins)>, Results<(outs)> {
+  let summary = "Terminator for OpenMP regions.";
+  let description = [{
----------------
summary starts with small letter for other constructs.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:106-107
+    operation.  These regions are not expected to return any value so the
+    terminator takes no operands. The terminator op returns control to the
+    enclosing op.
+  }];
----------------
Is the terminator Op required if the behaviour is just to return control to enclosing op? Hopefully the reviewers can help here.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:74
+
+  // p << ": " << op.type();
+}
----------------
Remove this?


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:26
+
+func @omp_parallel(%t : memref<i32>, %b : i1, %i : si32) -> () {
+  // CHECK: omp_parallel
----------------
For this test can we,
-> Add empty lines in between various checks for readability.
-> If the arguments are named to match what they mean it will be easier to read. (I agree that it can also mislead. :))


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:32
+    "omp.parallel"(%i, %t, %t, %t, %t) ({
+      omp.terminator
+    }) {operand_segment_sizes = dense<[0,1,1,1,1,1]>: vector<6xi32>} : (si32, memref<i32>, memref<i32>, memref<i32>, memref<i32>) -> ()
----------------
Can this be removed or made optional?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79410/new/

https://reviews.llvm.org/D79410