[mlir][OpenMP] OpenMP Synchronization Hints stored as IntegerAttr

[shraiysh]

Summary

hint-expression is an IntegerAttr, because it can be a combination of multiple values from the enum omp_sync_hint_t (Section 2.17.12 of OpenMP 5.0)

[shraiysh]

Removed minor unrelated test-case change.

[kiranchandramohan]

Thanks @shraiysh for the fix.

Would it be possible to keep the new representation but custom print or parse the hint as strings (since strings are more readable)?

[shraiysh]

It can be done and it certainly would be more readable, but one catch is that the frontend has to first convert an integer into such strings, which we will then again convert to the integer. Plus we would lose the assemblyFormat. Basically, the lowering would be something like this

#pragma omp critical hint(10) // C/C++ or Fortran

omp.critical hint(speculative) hint(contended) // MLIR

__kmpc_omp_critical_with_hints(..., 10) // LLVM IR

Please let me know if I should change it to this.

[kiranchandramohan]

In D111360#3049588, @shraiysh wrote:

It can be done and it certainly would be more readable, but one catch is that the frontend has to first convert an integer into such strings, which we will then again convert to the integer. Plus we would lose the assemblyFormat. Basically, the lowering would be something like this

#pragma omp critical hint(10) // C/C++ or Fortran

omp.critical hint(speculative) hint(contended) // MLIR

__kmpc_omp_critical_with_hints(..., 10) // LLVM IR

Please let me know if I should change it to this.

The builder (which is an interface for a frontend) can take in a hint as an integer, the parser and printer only has to deal with conversions.

For the assembly format you need a custom part only for the hint.

The format can also be hint(a,b).

[shraiysh]

Okay, that sounds good. I will do that.

[shraiysh]

Added custom printer and parser for hints as strings.

[ftynse]

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

378–379

Having an extra unit attr just for the sake of pretty-printing sounds obnoxious. We can just skip printing the hint altogether if it has the default (0) value. The only thing it will require is to have custom<SynchronizationHint>($hint) also print (or not) the "hint" keyword.

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

974

Please document top-level functions.

990

This will just ignore keywords that are not in this list, I would rather emit an error than silently discard input.

999

hint(none) works by accident because unknown keywords are ignored, hint(all_possible_hints) has exactly the same effect...

[shraiysh]

Addressed comments.

[ftynse]

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

1014

Nit: /// here and below for top-level comments.

[shraiysh]

Addressed comments. I do not have commit access, can someone please land this revision. Thank you.

[kiranchandramohan]

LGTM.

I will merge this soon. Will you be cherry-picking this patch to fir-dev?

[shraiysh]

Will you be cherry-picking this patch to fir-dev?

I am sorry for the delay, I missed this notification somehow, just saw it. I will cherry-pick this patch to fir-dev soon.

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

def CriticalDeclareOp : OpenMP_Op<"critical.declare", [Symbol]> {
   }];
 
   let arguments = (ins SymbolNameAttr:$sym_name);
 
   let assemblyFormat = "$sym_name attr-dict";
 }
 
 
-// TODO: Autogenerate this from OMP.td in llvm/include/Frontend
-def omp_sync_hint_none: I32EnumAttrCase<"none", 0>;
-def omp_sync_hint_uncontended: I32EnumAttrCase<"uncontended", 1>;
-def omp_sync_hint_contended: I32EnumAttrCase<"contended", 2>;
-def omp_sync_hint_nonspeculative: I32EnumAttrCase<"nonspeculative", 3>;
-def omp_sync_hint_speculative: I32EnumAttrCase<"speculative", 4>;
-
-def SyncHintKind: I32EnumAttr<"SyncHintKind", "OpenMP Sync Hint Kind",
-    [omp_sync_hint_none, omp_sync_hint_uncontended, omp_sync_hint_contended,
-     omp_sync_hint_nonspeculative, omp_sync_hint_speculative]> {
-  let cppNamespace = "::mlir::omp";
-  let stringToSymbolFnName = "ConvertToEnum";
-  let symbolToStringFnName = "ConvertToString";
-}
-
 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,
-                       OptionalAttr<SyncHintKind>:$hint);
+                       UnitAttr:$has_hint,
+                       DefaultValuedAttr<I32Attr, "0">:$hint);
 
   let regions = (region AnyRegion:$region);
 
   let assemblyFormat = [{
-    (`(` $name^ `)`)? (`hint` `(` $hint^ `)`)? $region attr-dict
+    (`(` $name^ `)`)? ( `hint` $has_hint^ `(` $hint `)` )? $region attr-dict
   }];
 
   let verifier = "return ::verifyCriticalOp(*this);";
 }
 
 //===----------------------------------------------------------------------===//
 // 2.17.2 barrier Construct
 //===----------------------------------------------------------------------===//

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

if (decl.getAccumulatorType() && decl.getAccumulatorType() != varType) {
              << ") to be the same type as reduction declaration ("
              << decl.getAccumulatorType() << ")";
     }
   }
 
   return success();
 }
 
+static LogicalResult verifySynchronizationHint(Operation *op, int32_t hint) {
+
+  auto bitn = [](int n, int idx) -> bool { return n & (1 << idx); };
+
+  bool uncontended = bitn(hint, 0);
+  bool contended = bitn(hint, 1);
+  bool nonspeculative = bitn(hint, 2);
+  bool speculative = bitn(hint, 3);
+
+  if (uncontended && contended)
+    return op->emitOpError() << "the hints omp_sync_hint_uncontended and "
+                                "omp_sync_hint_contended cannot be combined";
+  if (nonspeculative && speculative)
+    return op->emitOpError() << "the hints omp_sync_hint_nonspeculative and "
+                                "omp_sync_hint_speculative cannot be combined.";
+  return success();
+}
+
 static LogicalResult verifyCriticalOp(CriticalOp op) {
-  if (!op.name().hasValue() && op.hint().hasValue() &&
-      (op.hint().getValue() != SyncHintKind::none))
+
+  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 "
                                "hint(none) is specified";
 
   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 = nullptr;
-  if (criticalOp.hint().hasValue()) {
-    hint =
-        llvm::ConstantInt::get(llvm::Type::getInt32Ty(llvmContext),
-                               static_cast<int>(criticalOp.hint().getValue()));
-  } else {
-    hint = llvm::ConstantInt::get(llvm::Type::getInt32Ty(llvmContext), 0);
-  }
+  llvm::Constant *hint = llvm::ConstantInt::get(
+      llvm::Type::getInt32Ty(llvmContext), static_cast<int>(criticalOp.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

func @foo(%lb : index, %ub : index, %step : index, %mem : memref<1xf32>) {
   }
   return
 }
 
 // -----
 
 func @omp_critical1() -> () {
   // expected-error @below {{must specify a name unless the effect is as if hint(none) is specified}}
-  omp.critical hint(nonspeculative) {
+  omp.critical hint(4) {
     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) hint(8) {
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_critical() -> () {
+  // expected-error @below {{the hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined}}
+  omp.critical() hint(3) {
+    omp.terminator
+  }
+  return
+}
+
+// -----
+
+func @omp_critical() -> () {
+  // expected-error @below {{the hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined}}
+  omp.critical() hint(12) {
     omp.terminator
   }
   return
 }

mlir/test/Dialect/OpenMP/ops.mlir

 }
 
 // CHECK: omp.critical.declare
 // CHECK-LABEL: @mutex
 omp.critical.declare @mutex
 
 // CHECK-LABEL: omp_critical
 func @omp_critical() -> () {
+  // CHECK: omp.critical
   omp.critical {
     omp.terminator
   }
 
-  omp.critical(@mutex) hint(nonspeculative) {
+  // CHECK: omp.critical(@{{.*}}) hint(4)
+  omp.critical(@mutex) hint(4) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(1)
+  omp.critical(@mutex) hint(1) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(2)
+  omp.critical(@mutex) hint(2) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(4)
+  omp.critical(@mutex) hint(4) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(5)
+  omp.critical(@mutex) hint(5) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(6)
+  omp.critical(@mutex) hint(6) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(8)
+  omp.critical(@mutex) hint(8) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(9)
+  omp.critical(@mutex) hint(9) {
+    omp.terminator
+  }
+  // CHECK: omp.critical(@{{.*}}) hint(10)
+  omp.critical(@mutex) hint(10) {
     omp.terminator
   }
   return
 }

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

// 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) hint(2) {
   // CHECK: store
     llvm.store %xval, %x : !llvm.ptr<i32>
     omp.terminator
   }
   // CHECK: call void @__kmpc_end_critical({{.*}}critical_user_mutex.var{{.*}})
   llvm.return
 }