[MLIR][OpenMP] Shifted hint from CriticalOp to CriticalDeclareOp

[shraiysh]

Summary

According to the OpenMP 5.0 standard, names and hints of critical operation are
closely related. The following are the restrictions on them:

• Unless the effect is as if hint(omp_sync_hint_none) was specified, the critical construct must specify a name.
• If the hint clause is specified, each of the critical constructs with the same name must have a hint clause for which the hint-expression evaluates to the same value.

These restrictions will be enforced by design if the hint expression is a part
of the omp.critical.declare operation.

• Any operation with no "name" will be considered to have hint(omp_sync_hint_none).
• All the operations with the same "name" will have the same hint value.

[kiranchandramohan]

That is a very good observation @shraiysh. Thanks, LGTM.

[shraiysh]

Thank you @kiranchandramohan for the review. I will land this now.

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

def CriticalDeclareOp : OpenMP_Op<"critical.declare", [Symbol]> {
   let summary = "declares a named critical section.";
 
   let description = [{
     Declares a named critical section.
 
     The name can be used in critical constructs in the dialect.
   }];
 
-  let arguments = (ins SymbolNameAttr:$sym_name);
+  let arguments = (ins SymbolNameAttr:$sym_name,
+                       DefaultValuedAttr<I64Attr, "0">:$hint);
+
+  let assemblyFormat = [{
+    $sym_name custom<SynchronizationHint>($hint) attr-dict
+  }];
 
-  let assemblyFormat = "$sym_name attr-dict";
+  let verifier = "return verifyCriticalDeclareOp(*this);";
 }
 
 
 def CriticalOp : OpenMP_Op<"critical"> {
   let summary = "critical construct";
   let description = [{
     The critical construct imposes a restriction on the associated structured
     block (region) to be executed by only a single thread at a time.
   }];
 
-  let arguments = (ins OptionalAttr<FlatSymbolRefAttr>:$name,
-                       DefaultValuedAttr<I64Attr, "0">:$hint);
+  let arguments = (ins OptionalAttr<FlatSymbolRefAttr>:$name);
 
   let regions = (region AnyRegion:$region);
 
   let assemblyFormat = [{
-    (`(` $name^ `)`)? custom<SynchronizationHint>($hint) $region attr-dict
+    (`(` $name^ `)`)? $region attr-dict
   }];
 
   let verifier = "return ::verifyCriticalOp(*this);";
 }
 
 //===----------------------------------------------------------------------===//
 // 2.17.2 barrier Construct
 //===----------------------------------------------------------------------===//

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

 static LogicalResult verifyWsLoopOp(WsLoopOp op) {
   return verifyReductionVarList(op, op.reductions(), op.reduction_vars());
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for critical construct (2.17.1)
 //===----------------------------------------------------------------------===//
 
-static LogicalResult verifyCriticalOp(CriticalOp op) {
-
-  if (failed(verifySynchronizationHint(op, op.hint()))) {
-    return failure();
-  }
-  if (!op.name().hasValue() && (op.hint() != 0))
-    return op.emitOpError() << "must specify a name unless the effect is as if "
-                               "no hint is specified";
+static LogicalResult verifyCriticalDeclareOp(CriticalDeclareOp op) {
+  return verifySynchronizationHint(op, op.hint());
+
+static LogicalResult verifyCriticalOp(CriticalOp op) {
 
   if (op.nameAttr()) {
     auto symbolRef = op.nameAttr().cast<SymbolRefAttr>();
     auto decl =
         SymbolTable::lookupNearestSymbolFrom<CriticalDeclareOp>(op, symbolRef);
     if (!decl) {
       return op.emitOpError() << "expected symbol reference " << symbolRef
                               << " to point to a critical declaration";
     }
   }
 
   return success();
 }
 
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
 
   // TODO: Perform finalization actions for variables. This has to be
   // called for variables which have destructors/finalizers.
   auto finiCB = [&](InsertPointTy codeGenIP) {};
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(
       builder.saveIP(), builder.getCurrentDebugLocation());
   llvm::LLVMContext &llvmContext = moduleTranslation.getLLVMContext();
-  llvm::Constant *hint = llvm::ConstantInt::get(
-      llvm::Type::getInt32Ty(llvmContext), static_cast<int>(criticalOp.hint()));
+  llvm::Constant *hint = nullptr;
+
+  // If it has a name, it probably has a hint too.
+  if (criticalOp.nameAttr()) {
+    // The verifiers in OpenMP Dialect guarentee that all the pointers are
+    // non-null
+    auto symbolRef = criticalOp.nameAttr().cast<SymbolRefAttr>();
+    auto criticalDeclareOp =
+        SymbolTable::lookupNearestSymbolFrom<omp::CriticalDeclareOp>(criticalOp,
+                                                                     symbolRef);
+    hint = llvm::ConstantInt::get(llvm::Type::getInt32Ty(llvmContext),
+                                  static_cast<int>(criticalDeclareOp.hint()));
+  }
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createCritical(
       ompLoc, bodyGenCB, finiCB, criticalOp.name().getValueOr(""), hint));
   return success();
 }
 
 /// Returns a reduction declaration that corresponds to the given reduction
 /// operation in the given container. Currently only supports reductions inside
 /// WsLoopOp but can be easily extended.

mlir/test/Dialect/OpenMP/invalid.mlir

reduction(@add_f32 -> %mem : memref<1xf32>) {
     omp.reduction %2, %mem : memref<1xf32>
     omp.yield
   }
   return
 }
 
 // -----
 
-func @omp_critical1() -> () {
-  // expected-error @below {{must specify a name unless the effect is as if no hint is specified}}
-  omp.critical hint(nonspeculative) {
-    omp.terminator
-  }
-  return
-}
-
-// -----
-
 func @omp_critical2() -> () {
   // expected-error @below {{expected symbol reference @excl to point to a critical declaration}}
-  omp.critical(@excl) hint(speculative) {
+  omp.critical(@excl) {
     omp.terminator
   }
   return
 }
 
 // -----
 
-omp.critical.declare @mutex
-func @omp_critical() -> () {
-  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
-  omp.critical(@mutex) hint(uncontended, contended) {
-    omp.terminator
-  }
-  return
-}
+omp.critical.declare @mutex hint(uncontended, contended)
 
 // -----
 
-omp.critical.declare @mutex
-func @omp_critical() -> () {
-  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined}}
-  omp.critical(@mutex) hint(nonspeculative, speculative) {
-    omp.terminator
-  }
-  return
-}
+omp.critical.declare @mutex hint(nonspeculative, speculative)
 
 // -----
 
-omp.critical.declare @mutex
-func @omp_critica() -> () {
-  // expected-error @below {{invalid_hint is not a valid hint}}
-  omp.critical(@mutex) hint(invalid_hint) {
-    omp.terminator
-  }
-}
+omp.critical.declare @mutex hint(invalid_hint)

mlir/test/Dialect/OpenMP/ops.mlir

reduction(@add2_f32 -> %0 : memref<1xf32>) {
     %1 = arith.constant 2.0 : f32
     // CHECK: omp.reduction
     omp.reduction %1, %0 : memref<1xf32>
     omp.yield
   }
   return
 }
 
-// CHECK: omp.critical.declare
-// CHECK-LABEL: @mutex
-omp.critical.declare @mutex
+// CHECK: omp.critical.declare @mutex1 hint(uncontended)
+omp.critical.declare @mutex1 hint(uncontended)
+// CHECK: omp.critical.declare @mutex2 hint(contended)
+omp.critical.declare @mutex2 hint(contended)
+// CHECK: omp.critical.declare @mutex3 hint(nonspeculative)
+omp.critical.declare @mutex3 hint(nonspeculative)
+// CHECK: omp.critical.declare @mutex4 hint(speculative)
+omp.critical.declare @mutex4 hint(speculative)
+// CHECK: omp.critical.declare @mutex5 hint(uncontended, nonspeculative)
+omp.critical.declare @mutex5 hint(uncontended, nonspeculative)
+// CHECK: omp.critical.declare @mutex6 hint(contended, nonspeculative)
+omp.critical.declare @mutex6 hint(contended, nonspeculative)
+// CHECK: omp.critical.declare @mutex7 hint(uncontended, speculative)
+omp.critical.declare @mutex7 hint(uncontended, speculative)
+// CHECK: omp.critical.declare @mutex8 hint(contended, speculative)
+omp.critical.declare @mutex8 hint(contended, speculative)
+
 
 // CHECK-LABEL: omp_critical
 func @omp_critical() -> () {
   // CHECK: omp.critical
   omp.critical {
     omp.terminator
   }
 
-  // CHECK: omp.critical(@{{.*}}) hint(uncontended)
-  omp.critical(@mutex) hint(uncontended) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(contended)
-  omp.critical(@mutex) hint(contended) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(nonspeculative)
-  omp.critical(@mutex) hint(nonspeculative) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(uncontended, nonspeculative)
-  omp.critical(@mutex) hint(uncontended, nonspeculative) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(contended, nonspeculative)
-  omp.critical(@mutex) hint(nonspeculative, contended) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(speculative)
-  omp.critical(@mutex) hint(speculative) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(uncontended, speculative)
-  omp.critical(@mutex) hint(uncontended, speculative) {
-    omp.terminator
-  }
-  // CHECK: omp.critical(@{{.*}}) hint(contended, speculative)
-  omp.critical(@mutex) hint(speculative, contended) {
+  // CHECK: omp.critical(@{{.*}})
+  omp.critical(@mutex1) {
     omp.terminator
   }
   return
 }

mlir/test/Target/LLVMIR/openmp-llvm.mlir

omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(guided) {
    llvm.call @body(%iv) : (i64) -> ()
    omp.yield
  }
  llvm.return
 }
 
 // -----
 
-omp.critical.declare @mutex
+omp.critical.declare @mutex hint(contended)
 
 // CHECK-LABEL: @omp_critical
 llvm.func @omp_critical(%x : !llvm.ptr<i32>, %xval : i32) -> () {
-  // CHECK: call void @__kmpc_critical_with_hint({{.*}}critical_user_.var{{.*}}, i32 0)
+  // CHECK: call void @__kmpc_critical({{.*}}critical_user_.var{{.*}})
   // CHECK: br label %omp.critical.region
   // CHECK: omp.critical.region
   omp.critical {
   // CHECK: store
     llvm.store %xval, %x : !llvm.ptr<i32>
     omp.terminator
   }
   // CHECK: call void @__kmpc_end_critical({{.*}}critical_user_.var{{.*}})
 
   // CHECK: call void @__kmpc_critical_with_hint({{.*}}critical_user_mutex.var{{.*}}, i32 2)
   // CHECK: br label %omp.critical.region
   // CHECK: omp.critical.region
-  omp.critical(@mutex) hint(contended) {
+  omp.critical(@mutex) {
   // CHECK: store
     llvm.store %xval, %x : !llvm.ptr<i32>
     omp.terminator
   }
   // CHECK: call void @__kmpc_end_critical({{.*}}critical_user_mutex.var{{.*}})
   llvm.return
 }