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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

Generally LGTM. A couple nits and minor comments.



================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:961
+  auto cct = cancel_construct_val();
+  auto op = (*this)->getParentOp();
+
----------------
Same as below: please expand auto and can we change the name to parentOp because `op` is too generic.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:969
+  if ((cct == ClauseCancelConstructType::Parallel) &&
+      !(op_name.equals("omp.parallel")))
+    return emitOpError() << "cancel parallel must appear "
----------------
Same as below: string based matching should be avoided. It would be better to use `isa<ParallelOp>(parentOp)` instead.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:977
+                         << "inside a sections region";
+  // TODO : Add more when we support taskgroup and do/for.
+  return success();
----------------
do/for is `omp.wsloop`. We can add check for that too.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:985
+LogicalResult CancellationPointOp::verify() {
+  auto cct = cancel_construct_val();
+  auto op = (*this)->getParentOp();
----------------
nit: please expand `auto` for small type names and when the type is not obvious from the right hand side.
Ref: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:986
+  auto cct = cancel_construct_val();
+  auto op = (*this)->getParentOp();
+
----------------
nit: op is too generic. Can we please change the name to `parentOp`?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:994
+  if ((cct == ClauseCancelConstructType::Parallel) &&
+      !(op_name.equals("omp.parallel")))
+    return emitOpError() << "cancellation point parallel must appear "
----------------
string based matching should be avoided. It would be better to use `isa<ParallelOp>(parentOp)` instead.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:1016
+  // Test with optional operand; if_expr.
+  omp.parallel {
+    // CHECK: omp.cancel cancel_construct(parallel) if(%{{.*}})
----------------
Nit: please add tests for cancel construct under omp.sections, omp.section and omp.wsloop also.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123828