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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:57
 
+def ClauseDefaultPrivate : StrEnumAttrCase<"defprivate">;
+def ClauseDefaultFirstPrivate : StrEnumAttrCase<"deffirstprivate">;
----------------
nit: Can you add more documentation for what these correspond to?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:80
+
+def ParallelOp : OpenMP_Op<"parallel", [AttrSizedOperandSegments]>,
+  Arguments<(ins Optional<I1>:$if_expr_var,
----------------
Is there a logical ordering of some kind you can establish for this file, e.g., alphabetical?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:81
+def ParallelOp : OpenMP_Op<"parallel", [AttrSizedOperandSegments]>,
+  Arguments<(ins Optional<I1>:$if_expr_var,
+             Optional<AnySignedInteger>:$num_threads_var,
----------------
nit: Can you use `let arguments = (ins ...)` instead? The op definitions end up being much cleaner IMO.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:49
+  Type type;
+  if (parser.parseColon() || parser.parseType(type))
+    return failure();
----------------
nit: parseColonType


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:59
+  p << '(';
+  bool first = true;
+  for (auto value : op.getOperands()) {
----------------
Can you use llvm::interleaveComma instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79410