[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

380

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

962

Please document top-level functions.

978

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

1064

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

1002

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);
+                       DefaultValuedAttr<I64Attr, "0">:$hint);
 
   let regions = (region AnyRegion:$region);
 
   let assemblyFormat = [{
-    (`(` $name^ `)`)? (`hint` `(` $hint^ `)`)? $region attr-dict
+    (`(` $name^ `)`)? custom<SynchronizationHint>($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();
 }
 
+//===----------------------------------------------------------------------===//
+// Parser, printer and verifier for Synchronization Hint (2.17.12)
+//===----------------------------------------------------------------------===//
+
+/// Parses a Synchronization Hint clause. The value of hint is an integer
+/// which is a combination of different hints from `omp_sync_hint_t`.
+///
+/// hint-clause = `hint` `(` hint-value `)`
+static ParseResult parseSynchronizationHint(OpAsmParser &parser,
+                                            IntegerAttr &hintAttr) {
+  if (failed(parser.parseOptionalKeyword("hint"))) {
+    hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), 0);
+    return success();
+  }
+
+  if (failed(parser.parseLParen()))
+    return failure();
+  StringRef hintKeyword;
+  int64_t hint = 0;
+  do {
+    if (failed(parser.parseKeyword(&hintKeyword)))
+      return failure();
+    if (hintKeyword == "uncontended")
+      hint |= 1;
+    else if (hintKeyword == "contended")
+      hint |= 2;
+    else if (hintKeyword == "nonspeculative")
+      hint |= 4;
+    else if (hintKeyword == "speculative")
+      hint |= 8;
+    else
+      return parser.emitError(parser.getCurrentLocation())
+             << hintKeyword << " is not a valid hint";
+  } while (succeeded(parser.parseOptionalComma()));
+  if (failed(parser.parseRParen()))
+    return failure();
+  hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), hint);
+  return success();
+}
+
+/// Prints a Synchronization Hint clause
+static void printSynchronizationHint(OpAsmPrinter &p, Operation *op,
+                                     IntegerAttr hintAttr) {
+  int64_t hint = hintAttr.getInt();
+
+  if (hint == 0)
+    return;
+
+  // Helper function to get n-th bit from the right end of `value`
+  auto bitn = [](int value, int n) -> bool { return value & (1 << n); };
+
+  bool uncontended = bitn(hint, 0);
+  bool contended = bitn(hint, 1);
+  bool nonspeculative = bitn(hint, 2);
+  bool speculative = bitn(hint, 3);
+
+  SmallVector<StringRef> hints;
+  if (uncontended)
+    hints.push_back("uncontended");
+  if (contended)
+    hints.push_back("contended");
+  if (nonspeculative)
+    hints.push_back("nonspeculative");
+  if (speculative)
+    hints.push_back("speculative");
+
+  p << "hint(";
+  llvm::interleaveComma(hints, p);
+  p << ")";
+}
+
+/// Verifies a synchronization hint clause
+static LogicalResult verifySynchronizationHint(Operation *op, int32_t hint) {
+
+  // Helper function to get n-th bit from the right end of `value`
+  auto bitn = [](int value, int n) -> bool { return value & (1 << n); };
+
+  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();
+}
+
+//===----------------------------------------------------------------------===//
+// Verifier for critical construct (2.17.1)
+//===----------------------------------------------------------------------===//
+
 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";
+                               "no hint 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

reduction(@add_f32 -> %mem : memref<1xf32>) {
     omp.yield
   }
   return
 }
 
 // -----
 
 func @omp_critical1() -> () {
-  // expected-error @below {{must specify a name unless the effect is as if hint(none) is specified}}
+  // 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.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
+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
+func @omp_critica() -> () {
+  // expected-error @below {{invalid_hint is not a valid hint}}
+  omp.critical(@mutex) hint(invalid_hint) {
+    omp.terminator
+  }
+}

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
   }
 
+  // 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) {
+    omp.terminator
+  }
   return
 }