[mlir][OpenMP] Add omp.cancel and omp.cancellationpoint.

[kiranchandramohan]

flang/lib/Semantics/check-omp-structure.cpp

1809

On a first look, it looks like we have a construct here instead of a clause. Would CancelConstructType be a better name?

mlir/test/Dialect/OpenMP/ops.mlir

1278–1301

There are a few restrictions regarding closely nested constructs. Some of the above might not be valid usage. Could you try implementing them?

[raghavendhra]

flang/lib/Semantics/check-omp-structure.cpp

1809

If I change CancelConstruct to CancelConstructType, I am getting

llvm-project/flang/lib/Semantics/check-omp-structure.cpp:1808:21: error: ‘CancelConstructType’ in ‘struct Fortran::parser::OmpClause’ does not name a type
1808 | CHECK_SIMPLE_CLAUSE(CancelConstructType, OMPC_cancel_construct)

Also, it looks like code in flang has been using CancelConstruct in multiple places.

mlir/test/Dialect/OpenMP/ops.mlir

1278–1301

Something like this?

func @omp_cancel(%if_cond : i1) -> () {
  // Test with optional operand; if_expr.
  omp.parallel {
    // CHECK: omp.cancel cancel_construct(parallel) if(%{{.*}})
    omp.cancel cancel_construct(parallel) if(%if_cond)
    // CHECK: omp.terminator
    omp.terminator
  }
  return
}

[clementval]

flang/lib/Semantics/check-omp-structure.cpp

1809

You have the error because you have to update the name in OMP.td first.

[raghavendhra]

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

[kiranchandramohan]

@raghavendhra Apologies for not being clear. The change request was to add a verifier to the operation to issue errors where the closely nested construct restrictions do not hold.

As per the standard, for example, the following usages are not allowed.

!$omp master
!$omp cancel parallel
!$omp end master

!$omp cancel parallel

Could you go through the restrictions of cancel and cancellation point (the restriction on nesting of constructs) and implement these in the verifier? You could use the Reduction Op's verifier for reference. We probably have to check the parent Op and ensure it meets the restrictions specified in the standard.
https://github.com/llvm/llvm-project/blob/76410040b9f391185c7df48c14519860e1cf75e5/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L1074
https://github.com/llvm/llvm-project/blob/76410040b9f391185c7df48c14519860e1cf75e5/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L713

[raghavendhra]

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

[raghavendhra]

In D123828#3458727, @kiranchandramohan wrote:

@raghavendhra Apologies for not being clear. The change request was to add a verifier to the operation to issue errors where the closely nested construct restrictions do not hold.

As per the standard, for example, the following usages are not allowed.

!$omp master
!$omp cancel parallel
!$omp end master

!$omp cancel parallel

Could you go through the restrictions of cancel and cancellation point (the restriction on nesting of constructs) and implement these in the verifier? You could use the Reduction Op's verifier for reference. We probably have to check the parent Op and ensure it meets the restrictions specified in the standard.
https://github.com/llvm/llvm-project/blob/76410040b9f391185c7df48c14519860e1cf75e5/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L1074
https://github.com/llvm/llvm-project/blob/76410040b9f391185c7df48c14519860e1cf75e5/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L713

Thank you so much for more insight. I added verifier and tests to invalid.mlir. Please review my latest changes.

[shraiysh]

Generally LGTM. A couple nits and minor comments.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

959

Same as below: please expand auto and can we change the name to parentOp because op is too generic.

967

Same as below: string based matching should be avoided. It would be better to use isa<ParallelOp>(parentOp) instead.

975

do/for is omp.wsloop. We can add check for that too.

983

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

984

nit: op is too generic. Can we please change the name to parentOp?

992

string based matching should be avoided. It would be better to use isa<ParallelOp>(parentOp) instead.

mlir/test/Dialect/OpenMP/ops.mlir

1280

Nit: please add tests for cancel construct under omp.sections, omp.section and omp.wsloop also.

[raghavendhra]

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

[kiranchandramohan]

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

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

961

Nit: Might be better to use braces here since the return message is in two lines. Same for cancellation point as well.

962

Nit: an region -> a region

966

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.

970

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.

[peixin]

Mostly LGTM.

flang/lib/Semantics/check-omp-structure.cpp

1809

Niy: Would CancellationConstructType be better? Both cancel construct and cancellation point construct are cancellation constructs.

llvm/include/llvm/Frontend/OpenMP/OMP.td

176

Nit

[raghavendhra]

Addressed comments

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

[peixin]

llvm/include/llvm/Frontend/OpenMP/OMP.td

174

Can you update them all if you use CancellationConstructType for the keyword?
OMPC_CancelConstructType -> OMPC_CancellationConstructType
OMP_CANCEL_CONSTRUCT_Parallel -> OMP_CANCELLATION_CONSTRUCT_Parallel
...

[raghavendhra]

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

[kiranchandramohan]

LGTM. Please check the CI failure (probably a clang-format issue). Please wait for @shraiysh & @peixin.

[peixin]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

1012

Nit: cancel_construct -> cancellation_construct_type or cancellation_type

1028

Nit: cancel_construct_val -> cancellation_construct_type_val or cancellation_type_val

1029

Nit: cancel_construct -> cancellation_construct_type or cancellation_type

[shraiysh]

LGTM. Thanks!

[raghavendhra]

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

[raghavendhra]

Fixed git-clang-format

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

[raghavendhra]

Rebasing with main

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

[raghavendhra]

Rebasing it to main to land the patch.

[raghavendhra]

Rebasing to main before landing.

[MaskRay]

ninja clang had a -Wswitch warning when the host compiler is clang. It was something like:

clang/lib/CodeGen/CGStmtOpenMP.cpp:6166:11: error: enumeration value 'OMPC_cancellation_construct_type' not handled in switch [-Werror,-Wswitch]

I have fixed it in 4d34c4e0e67d321ac6b52f787648a68ea6d267c0 but please pay attention to warnings in the future :)

flang/lib/Semantics/check-omp-structure.cpp

 CHECK_SIMPLE_CLAUSE(Filter, OMPC_filter)
 CHECK_SIMPLE_CLAUSE(When, OMPC_when)
 CHECK_SIMPLE_CLAUSE(AdjustArgs, OMPC_adjust_args)
 CHECK_SIMPLE_CLAUSE(AppendArgs, OMPC_append_args)
 CHECK_SIMPLE_CLAUSE(MemoryOrder, OMPC_memory_order)
 CHECK_SIMPLE_CLAUSE(Bind, OMPC_bind)
 CHECK_SIMPLE_CLAUSE(Align, OMPC_align)
 CHECK_SIMPLE_CLAUSE(Compare, OMPC_compare)
+CHECK_SIMPLE_CLAUSE(CancellationConstructType, OMPC_cancellation_construct_type)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
 CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
 CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
 CHECK_REQ_SCALAR_INT_CLAUSE(Device, OMPC_device)

llvm/include/llvm/Frontend/OpenMP/OMP.td

let allowedClauseValues = [
     OMP_MEMORY_ORDER_AcqRel,
     OMP_MEMORY_ORDER_Acquire,
     OMP_MEMORY_ORDER_Release,
     OMP_MEMORY_ORDER_Relaxed,
     OMP_MEMORY_ORDER_Default
   ];
 }
 
+def OMP_CANCELLATION_CONSTRUCT_Parallel : ClauseVal<"parallel", 1, 1> {}
+def OMP_CANCELLATION_CONSTRUCT_Loop : ClauseVal<"loop", 2, 1> {}
+def OMP_CANCELLATION_CONSTRUCT_Sections : ClauseVal<"sections", 3, 1> {}
+def OMP_CANCELLATION_CONSTRUCT_Taskgroup : ClauseVal<"taskgroup", 4, 1> {}
+def OMP_CANCELLATION_CONSTRUCT_None : ClauseVal<"none", 5, 0> {
+  let isDefault = 1;
+}
+
+def OMPC_CancellationConstructType : Clause<"cancellation_construct_type"> {
+  let enumClauseValue = "CancellationConstructType";
+  let allowedClauseValues = [
+    OMP_CANCELLATION_CONSTRUCT_Parallel,
+    OMP_CANCELLATION_CONSTRUCT_Loop,
+    OMP_CANCELLATION_CONSTRUCT_Sections,
+    OMP_CANCELLATION_CONSTRUCT_Taskgroup,
+    OMP_CANCELLATION_CONSTRUCT_None
+  ];
+}
+
 def OMPC_Ordered : Clause<"ordered"> {
   let clangClass = "OMPOrderedClause";
   let flangClass = "ScalarIntConstantExpr";
   let isValueOptional = true;
 }
 def OMPC_NoWait : Clause<"nowait"> {
   let clangClass = "OMPNowaitClause";
 }

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

def ThreadprivateOp : OpenMP_Op<"threadprivate"> {
   let arguments = (ins OpenMP_PointerLikeType:$sym_addr);
   let results = (outs OpenMP_PointerLikeType:$tls_addr);
   let assemblyFormat = [{
     $sym_addr `:` type($sym_addr) `->` type($tls_addr) attr-dict
   }];
 }
 
 //===----------------------------------------------------------------------===//
+// 2.18.1 Cancel Construct
+//===----------------------------------------------------------------------===//
+def CancelOp : OpenMP_Op<"cancel"> {
+  let summary = "cancel directive";
+  let description = [{
+    The cancel construct activates cancellation of the innermost enclosing
+    region of the type specified.
+  }];
+  let arguments = (ins CancellationConstructTypeAttr:$cancellation_construct_type_val,
+                       Optional<I1>:$if_expr);
+  let assemblyFormat = [{ `cancellation_construct_type` `(`
+                          custom<ClauseAttr>($cancellation_construct_type_val) `)`
+                          ( `if` `(` $if_expr^ `)` )? attr-dict}];
+  let hasVerifier = 1;
+}
+
+//===----------------------------------------------------------------------===//
+// 2.18.2 Cancellation Point Construct
+//===----------------------------------------------------------------------===//
+def CancellationPointOp : OpenMP_Op<"cancellationpoint"> {
+  let summary = "cancellation point directive";
+  let description = [{
+    The cancellation point construct introduces a user-defined cancellation
+    point at which implicit or explicit tasks check if cancellation of the
+    innermost enclosing region of the type specified has been activated.
+  }];
+  let arguments = (ins CancellationConstructTypeAttr:$cancellation_construct_type_val);
+  let assemblyFormat = [{ `cancellation_construct_type` `(`
+                           custom<ClauseAttr>($cancellation_construct_type_val) `)`
+                           attr-dict}];
+  let hasVerifier = 1;
+}
+
+//===----------------------------------------------------------------------===//
 // 2.19.5.7 declare reduction Directive
 //===----------------------------------------------------------------------===//
 
 def ReductionDeclareOp : OpenMP_Op<"reduction.declare", [Symbol]> {
   let summary = "declares a reduction kind";
 
   let description = [{
     Declares an OpenMP reduction kind. This requires two mandatory and one

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

return firstReadStmt.emitError()
               "second operation";
 
   if (getFirstOp()->getAttr("hint_val") || getSecondOp()->getAttr("hint_val"))
     return emitOpError(
         "operations inside capture region must not have hint clause");
   return success();
 }
 
+//===----------------------------------------------------------------------===//
+// Verifier for CancelOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult CancelOp::verify() {
+  ClauseCancellationConstructType cct = cancellation_construct_type_val();
+  Operation *parentOp = (*this)->getParentOp();
+
+  if (!parentOp) {
+    return emitOpError() << "must be used within a region supporting "
+                            "cancel directive";
+  }
+
+  if ((cct == ClauseCancellationConstructType::Parallel) &&
+      !isa<ParallelOp>(parentOp)) {
+    return emitOpError() << "cancel parallel must appear "
+                         << "inside a parallel region";
+  } else if (cct == ClauseCancellationConstructType::Loop) {
+    if (!isa<WsLoopOp>(parentOp)) {
+      return emitOpError() << "cancel loop must appear "
+                           << "inside a worksharing-loop region";
+    } else {
+      if (cast<WsLoopOp>(parentOp).nowaitAttr()) {
+        return emitError() << "A worksharing construct that is canceled "
+                           << "must not have a nowait clause";
+      } else if (cast<WsLoopOp>(parentOp).ordered_valAttr()) {
+        return emitError() << "A worksharing construct that is canceled "
+                           << "must not have an ordered clause";
+      }
+    }
+  } else if (cct == ClauseCancellationConstructType::Sections) {
+    if (!(isa<SectionsOp>(parentOp) || isa<SectionOp>(parentOp))) {
+      return emitOpError() << "cancel sections must appear "
+                           << "inside a sections region";
+    }
+    if (parentOp->getParentOp() && isa<SectionsOp>(parentOp->getParentOp()) &&
+        cast<SectionsOp>(parentOp->getParentOp()).nowaitAttr()) {
+      return emitError() << "A sections construct that is canceled "
+                         << "must not have a nowait clause";
+    }
+  }
+  // TODO : Add more when we support taskgroup.
+  return success();
+}
+//===----------------------------------------------------------------------===//
+// Verifier for CancelOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult CancellationPointOp::verify() {
+  ClauseCancellationConstructType cct = cancellation_construct_type_val();
+  Operation *parentOp = (*this)->getParentOp();
+
+  if (!parentOp) {
+    return emitOpError() << "must be used within a region supporting "
+                            "cancellation point directive";
+  }
+
+  if ((cct == ClauseCancellationConstructType::Parallel) &&
+      !(isa<ParallelOp>(parentOp))) {
+    return emitOpError() << "cancellation point parallel must appear "
+                         << "inside a parallel region";
+  } else if ((cct == ClauseCancellationConstructType::Loop) &&
+             !isa<WsLoopOp>(parentOp)) {
+    return emitOpError() << "cancellation point loop must appear "
+                         << "inside a worksharing-loop region";
+  } else if ((cct == ClauseCancellationConstructType::Sections) &&
+             !(isa<SectionsOp>(parentOp) || isa<SectionOp>(parentOp))) {
+    return emitOpError() << "cancellation point sections must appear "
+                         << "inside a sections region";
+  }
+  // TODO : Add more when we support taskgroup.
+  return success();
+}
+
 #define GET_ATTRDEF_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOpsAttributes.cpp.inc"
 
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"

mlir/test/Dialect/OpenMP/invalid.mlir

func.func @omp_task(%mem: memref<1xf32>) {
   omp.task in_reduction(@add_i32 -> %mem : memref<1xf32>) {
     // CHECK: "test.foo"() : () -> ()
     "test.foo"() : () -> ()
     // CHECK: omp.terminator
     omp.terminator
   }
   return
 }
+
+// -----
+
+func @omp_cancel() {
+  omp.sections {
+    // expected-error @below {{cancel parallel must appear inside a parallel region}}
+    omp.cancel cancellation_construct_type(parallel)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancel1() {
+  omp.parallel {
+    // expected-error @below {{cancel sections must appear inside a sections region}}
+    omp.cancel cancellation_construct_type(sections)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancel2() {
+  omp.sections {
+    // expected-error @below {{cancel loop must appear inside a worksharing-loop region}}
+    omp.cancel cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancel3(%arg1 : i32, %arg2 : i32, %arg3 : i32) -> () {
+  omp.wsloop nowait
+    for (%0) : i32 = (%arg1) to (%arg2) step (%arg3) {
+    // expected-error @below {{A worksharing construct that is canceled must not have a nowait clause}}
+    omp.cancel cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancel4(%arg1 : i32, %arg2 : i32, %arg3 : i32) -> () {
+  omp.wsloop ordered(1)
+    for (%0) : i32 = (%arg1) to (%arg2) step (%arg3) {
+    // expected-error @below {{A worksharing construct that is canceled must not have an ordered clause}}
+    omp.cancel cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancel5() -> () {
+  omp.sections nowait {
+    omp.section {
+      // expected-error @below {{A sections construct that is canceled must not have a nowait clause}}
+      omp.cancel cancellation_construct_type(sections)
+      omp.terminator
+    }
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancellationpoint() {
+  omp.sections {
+    // expected-error @below {{cancellation point parallel must appear inside a parallel region}}
+    omp.cancellationpoint cancellation_construct_type(parallel)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancellationpoint1() {
+  omp.parallel {
+    // expected-error @below {{cancellation point sections must appear inside a sections region}}
+    omp.cancellationpoint cancellation_construct_type(sections)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_cancellationpoint2() {
+  omp.sections {
+    // expected-error @below {{cancellation point loop must appear inside a worksharing-loop region}}
+    omp.cancellationpoint cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}

mlir/test/Dialect/OpenMP/ops.mlir

omp.parallel  {
     %5 = omp.threadprivate %3 : !llvm.ptr<i32> -> !llvm.ptr<i32>
     llvm.store %1, %5 : !llvm.ptr<i32>
     omp.terminator
   }
   llvm.store %2, %4 : !llvm.ptr<i32>
   return
 }
 
 llvm.mlir.global internal @_QFsubEx() : i32
+
+func @omp_cancel_parallel(%if_cond : i1) -> () {
+  // Test with optional operand; if_expr.
+  omp.parallel {
+    // CHECK: omp.cancel cancellation_construct_type(parallel) if(%{{.*}})
+    omp.cancel cancellation_construct_type(parallel) if(%if_cond)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancel_wsloop(%lb : index, %ub : index, %step : index) {
+  omp.wsloop
+  for (%iv) : index = (%lb) to (%ub) step (%step) {
+    // CHECK: omp.cancel cancellation_construct_type(loop)
+    omp.cancel cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancel_sections() -> () {
+  omp.sections {
+    omp.section {
+      // CHECK: omp.cancel cancellation_construct_type(sections)
+      omp.cancel cancellation_construct_type(sections)
+      omp.terminator
+    }
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancellationpoint_parallel() -> () {
+  omp.parallel {
+    // CHECK: omp.cancellationpoint cancellation_construct_type(parallel)
+    omp.cancellationpoint cancellation_construct_type(parallel)
+    // CHECK: omp.cancel cancellation_construct_type(parallel)
+    omp.cancel cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancellationpoint_wsloop(%lb : index, %ub : index, %step : index) {
+  omp.wsloop
+  for (%iv) : index = (%lb) to (%ub) step (%step) {
+    // CHECK: omp.cancellationpoint cancellation_construct_type(loop)
+    omp.cancellationpoint cancellation_construct_type(loop)
+    // CHECK: omp.cancel cancellation_construct_type(loop)
+    omp.cancel cancellation_construct_type(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancellationpoint_sections() -> () {
+  omp.sections {
+    omp.section {
+      // CHECK: omp.cancellationpoint cancellation_construct_type(sections)
+      omp.cancellationpoint cancellation_construct_type(sections)
+      // CHECK: omp.cancel cancellation_construct_type(sections)
+      omp.cancel cancellation_construct_type(sections)
+      omp.terminator
+    }
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}