[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 ↗

(On Diff #424025)

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

969 ↗

(On Diff #424025)

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

977 ↗

(On Diff #424025)

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

985 ↗

(On Diff #424025)

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 ↗

(On Diff #424025)

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

994 ↗

(On Diff #424025)

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 ↗

(On Diff #424253)

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

964 ↗

(On Diff #424253)

Nit: an region -> a region

968 ↗

(On Diff #424253)

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 ↗

(On Diff #424253)

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(CancelConstruct, OMPC_cancel_construct)
 
 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_CancelConstruct : Clause<"cancel_construct"> {
+    let enumClauseValue = "CancelConstructKind";
+    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 CancelConstructKindAttr:$cancel_construct_val,
+                       Optional<I1>:$if_expr);
+  let assemblyFormat = [{ `cancel_construct` `(`
+                          custom<ClauseAttr>($cancel_construct_val) `)`
+                          ( `if` `(` $if_expr^ `)` )? attr-dict}];
+}
+
+//===----------------------------------------------------------------------===//
+// 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 CancelConstructKindAttr:$cancel_construct_val);
+  let assemblyFormat = [{ `cancel_construct` `(`
+                           custom<ClauseAttr>($cancel_construct_val) `)`
+                           attr-dict}];
+}
+
+//===----------------------------------------------------------------------===//
 // 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/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
 }
 
+func @omp_cancel(%if_cond : i1) -> () {
+  // Test with optional operand; if_expr.
+  // CHECK: omp.cancel cancel_construct(parallel) if(%{{.*}})
+  omp.cancel cancel_construct(parallel) if(%if_cond)
+  // CHECK: omp.cancel cancel_construct(loop)
+  omp.cancel cancel_construct(loop)
+  // CHECK: omp.cancel cancel_construct(sections)
+  omp.cancel cancel_construct(sections)
+  // CHECK: omp.cancel cancel_construct(taskgroup)
+  omp.cancel cancel_construct(taskgroup)
+  return
+}
+
+func @omp_cancellationpoint() -> () {
+  // CHECK: omp.cancellationpoint cancel_construct(parallel)
+  omp.cancellationpoint cancel_construct(parallel)
+  // CHECK: omp.cancellationpoint cancel_construct(loop)
+  omp.cancellationpoint cancel_construct(loop)
+  // CHECK: omp.cancellationpoint cancel_construct(sections)
+  omp.cancellationpoint cancel_construct(sections)
+  // CHECK: omp.cancellationpoint cancel_construct(taskgroup)
+  omp.cancellationpoint cancel_construct(taskgroup)
+  return
+}
+
 llvm.mlir.global internal @_QFsubEx() : i32