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

[kiranchandramohan]

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

1808

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

1014–1037

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

1808

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

1014–1037

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

1808

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

961

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

969

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

977

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

985

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

986

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

994

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

mlir/test/Dialect/OpenMP/ops.mlir

1016

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

963

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

964

Nit: an region -> a region

968

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.

972

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

1808

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

1011

Nit: cancel_construct -> cancellation_construct_type or cancellation_type

1027

Nit: cancel_construct_val -> cancellation_construct_type_val or cancellation_type_val

1028

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(CancelConstructType, OMPC_cancel_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_CANCEL_CONSTRUCT_Parallel : ClauseVal<"parallel", 1, 1> {}
+def OMP_CANCEL_CONSTRUCT_Loop : ClauseVal<"loop", 2, 1> {}
+def OMP_CANCEL_CONSTRUCT_Sections : ClauseVal<"sections", 3, 1> {}
+def OMP_CANCEL_CONSTRUCT_Taskgroup : ClauseVal<"taskgroup", 4, 1> {}
+def OMP_CANCEL_CONSTRUCT_None : ClauseVal<"none", 5, 0> {
+  let isDefault = 1;
+}
+
+def OMPC_CancelConstructType : Clause<"cancel_construct_type"> {
+    let enumClauseValue = "CancelConstructType";
+    let allowedClauseValues = [
+    OMP_CANCEL_CONSTRUCT_Parallel,
+    OMP_CANCEL_CONSTRUCT_Loop,
+    OMP_CANCEL_CONSTRUCT_Sections,
+    OMP_CANCEL_CONSTRUCT_Taskgroup,
+    OMP_CANCEL_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 CancelConstructTypeAttr:$cancel_construct_val,
+                       Optional<I1>:$if_expr);
+  let assemblyFormat = [{ `cancel_construct` `(`
+                          custom<ClauseAttr>($cancel_construct_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 CancelConstructTypeAttr:$cancel_construct_val);
+  let assemblyFormat = [{ `cancel_construct` `(`
+                           custom<ClauseAttr>($cancel_construct_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

LogicalResult AtomicCaptureOp::verifyRegions() {
   if (firstReadStmt && secondWriteStmt &&
       firstReadStmt.x() != secondWriteStmt.address())
     return firstReadStmt.emitError()
            << "captured variable in omp.atomic.read must be updated in "
               "second operation";
   return success();
 }
 
+//===----------------------------------------------------------------------===//
+// Verifier for CancelOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult CancelOp::verify() {
+  ClauseCancelConstructType cct = cancel_construct_val();
+  Operation* parentOp = (*this)->getParentOp();
+
+  if (!parentOp)
+    return emitOpError() << "must be used within an region supporting "
+                            "cancel directive";
+
+  if ((cct == ClauseCancelConstructType::Parallel) &&
+      !isa<ParallelOp>(parentOp))
+    return emitOpError() << "cancel parallel must appear "
+                         << "inside a parallel region";
+  else if ((cct == ClauseCancelConstructType::Loop) &&
+           !isa<WsLoopOp>(parentOp))
+    return emitOpError() << "cancel loop must appear "
+                         << "inside a worksharing-loop region";
+  else if ((cct == ClauseCancelConstructType::Sections) &&
+           !(isa<SectionsOp>(parentOp) ||
+             isa<SectionOp>(parentOp)))
+    return emitOpError() << "cancel sections must appear "
+                         << "inside a sections region";
+  // TODO : Add more when we support taskgroup.
+  return success();
+}
+//===----------------------------------------------------------------------===//
+// Verifier for CancelOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult CancellationPointOp::verify() {
+  ClauseCancelConstructType cct = cancel_construct_val();
+  Operation* parentOp = (*this)->getParentOp();
+
+  if (!parentOp)
+    return emitOpError() << "must be used within an region supporting "
+                            "cancellation point directive";
+
+  if ((cct == ClauseCancelConstructType::Parallel) &&
+      !(isa<ParallelOp>(parentOp)))
+    return emitOpError() << "cancellation point parallel must appear "
+                         << "inside a parallel region";
+  else if ((cct == ClauseCancelConstructType::Loop) &&
+           !isa<WsLoopOp>(parentOp))
+    return emitOpError() << "cancellation point loop must appear "
+                         << "inside a worksharing-loop region";
+  else if ((cct == ClauseCancelConstructType::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 @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 cancel_construct(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 cancel_construct(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 cancel_construct(loop)
+    // 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 cancel_construct(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 cancel_construct(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 cancel_construct(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 cancel_construct(parallel) if(%{{.*}})
+    omp.cancel cancel_construct(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 cancel_construct(loop)
+    omp.cancel cancel_construct(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancel_sections() -> () {
+  omp.sections {
+    omp.section {
+      // CHECK: omp.cancel cancel_construct(sections)
+      omp.cancel cancel_construct(sections)
+      omp.terminator
+    }
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancellationpoint_parallel() -> () {
+  omp.parallel {
+    // CHECK: omp.cancellationpoint cancel_construct(parallel)
+    omp.cancellationpoint cancel_construct(parallel)
+    // CHECK: omp.cancel cancel_construct(parallel)
+    omp.cancel cancel_construct(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 cancel_construct(loop)
+    omp.cancellationpoint cancel_construct(loop)
+    // CHECK: omp.cancel cancel_construct(loop)
+    omp.cancel cancel_construct(loop)
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}
+
+func @omp_cancellationpoint_sections() -> () {
+  omp.sections {
+    omp.section {
+      // CHECK: omp.cancellationpoint cancel_construct(sections)
+      omp.cancellationpoint cancel_construct(sections)
+      // CHECK: omp.cancel cancel_construct(sections)
+      omp.cancel cancel_construct(sections)
+      omp.terminator
+    }
+    // CHECK: omp.terminator
+    omp.terminator
+  }
+  return
+}