[PATCH] D123828: [mlir][OpenMP] Add omp.cancel and omp.cancellationpoint.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks for making the changes. This looks mostly good. A few more comments.



================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:963
+
+  if (!parentOp)
+    return emitOpError() << "must be used within an region supporting "
----------------
Nit: Might be better to use braces here since the return message is in two lines. Same for cancellation point as well.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:964
+  if (!parentOp)
+    return emitOpError() << "must be used within an region supporting "
+                            "cancel directive";
----------------
Nit: an region -> a region


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:968
+  if ((cct == ClauseCancelConstructType::Parallel) &&
+      !isa<ParallelOp>(parentOp))
+    return emitOpError() << "cancel parallel must appear "
----------------
Nit: Might be better to use braces in this entire if-else construct since some of them are in two lines. Same for cancellation point as well.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:972
+  else if ((cct == ClauseCancelConstructType::Loop) &&
+           !isa<WsLoopOp>(parentOp))
+    return emitOpError() << "cancel loop must appear "
----------------
Can you add the following two as well?
A worksharing construct that is canceled must not have a nowait clause.
A worksharing-loop construct that is canceled must not have an ordered clause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123828