[MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036

skatrak · 2024-10-29T11:48:19Z

This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom ErrorInfo subclass.

Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom `ErrorInfo` subclass. Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

llvmbot · 2024-10-29T11:48:53Z

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Sergio Afonso (skatrak)

Changes

This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom ErrorInfo subclass.

Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

Patch is 31.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114036.diff

3 Files Affected:

(modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+1-7)
(modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (+102-63)
(added) mlir/test/Target/LLVMIR/openmp-todo.mlir (+512)

diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index e1df647d6a3c71..4a27a5ed8eb74b 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2499,13 +2499,7 @@ void OrderedRegionOp::build(OpBuilder &builder, OperationState &state,
   OrderedRegionOp::build(builder, state, clauses.parLevelSimd);
 }
 
-LogicalResult OrderedRegionOp::verify() {
-  // TODO: The code generation for ordered simd directive is not supported yet.
-  if (getParLevelSimd())
-    return failure();
-
-  return verifyOrderedParent(**this);
-}
+LogicalResult OrderedRegionOp::verify() { return verifyOrderedParent(**this); }
 
 //===----------------------------------------------------------------------===//
 // TaskwaitOp
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index fc2f88b766f1c5..1df7b3c083a3a7 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -89,8 +89,54 @@ class OpenMPVarMappingStackFrame
 
   DenseMap<Value, llvm::Value *> mapping;
 };
+
+/// Custom error class to signal translation errors that don't need reporting,
+/// since encountering them will have already triggered relevant error messages.
+///
+/// For example, it should be used to trigger errors from within callbacks
+/// passed to the \see OpenMPIRBuilder when these errors resulted from the
+/// translation of their own regions. This unclutters the error log from
+/// redundant messages.
+class SilentTranslationError : public llvm::ErrorInfo<SilentTranslationError> {
+public:
+  void log(raw_ostream &) const override {
+    // Do not log anything.
+  }
+
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable(
+        "SilentTranslationError doesn't support ECError conversion");
+  }
+
+  // Used by ErrorInfo::classID.
+  static char ID;
+};
+
+char SilentTranslationError::ID = 0;
+
 } // namespace
 
+static LogicalResult handleError(llvm::Error error, Operation &op) {
+  LogicalResult result = success();
+  if (error) {
+    llvm::handleAllErrors(
+        std::move(error),
+        [&](const SilentTranslationError &) { result = failure(); },
+        [&](const llvm::ErrorInfoBase &err) {
+          result = op.emitError(err.message());
+        });
+  }
+  return result;
+}
+
+template <typename T>
+static LogicalResult handleError(llvm::Expected<T> &result, Operation &op) {
+  if (!result)
+    return handleError(result.takeError(), op);
+
+  return success();
+}
+
 /// Find the insertion point for allocas given the current insertion point for
 /// normal operations in the builder.
 static llvm::OpenMPIRBuilder::InsertPointTy
@@ -216,7 +262,7 @@ static llvm::Expected<llvm::BasicBlock *> convertOmpOpRegions(
     llvm::IRBuilderBase::InsertPointGuard guard(builder);
     if (failed(
             moduleTranslation.convertBlock(*bb, bb->isEntryBlock(), builder)))
-      return llvm::createStringError("failed region translation");
+      return llvm::make_error<SilentTranslationError>();
 
     // Special handling for `omp.yield` and `omp.terminator` (we may have more
     // than one): they return the control to the parent OpenMP dialect operation
@@ -296,8 +342,8 @@ convertOmpMasked(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMasked(ompLoc, bodyGenCB,
                                                          finiCB, filterVal);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -326,8 +372,8 @@ convertOmpMaster(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMaster(ompLoc, bodyGenCB,
                                                          finiCB);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -373,8 +419,8 @@ convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createCritical(
           ompLoc, bodyGenCB, finiCB, criticalOp.getName().value_or(""), hint);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -481,9 +527,8 @@ static LogicalResult inlineConvertOmpRegions(
   llvm::Expected<llvm::BasicBlock *> continuationBlock =
       convertOmpOpRegions(region, blockName, builder, moduleTranslation, &phis);
 
-  if (!continuationBlock)
-    return region.getParentOp()->emitError(
-        llvm::toString(continuationBlock.takeError()));
+  if (failed(handleError(continuationBlock, *region.getParentOp())))
+    return failure();
 
   if (continuationBlockArgs)
     llvm::append_range(*continuationBlockArgs, phis);
@@ -605,7 +650,7 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
 
   // TODO: The code generation for ordered simd directive is not supported yet.
   if (orderedRegionOp.getParLevelSimd())
-    return failure();
+    return opInst.emitError("unhandled clauses for translation to LLVM IR");
 
   auto bodyGenCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
     // OrderedOp has only one region associated with it.
@@ -625,8 +670,8 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createOrderedThreadsSimd(
           ompLoc, bodyGenCB, finiCB, !orderedRegionOp.getParLevelSimd());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -831,8 +876,8 @@ static LogicalResult createReductionsAndCleanup(
       ompBuilder->createReductions(builder.saveIP(), allocaIP, reductionInfos,
                                    isByRef, op.getNowait());
 
-  if (!contInsertPoint)
-    return op.emitError(llvm::toString(contInsertPoint.takeError()));
+  if (failed(handleError(contInsertPoint, *op)))
+    return failure();
 
   if (!contInsertPoint->getBlock())
     return op->emitOpError() << "failed to convert reductions";
@@ -840,8 +885,8 @@ static LogicalResult createReductionsAndCleanup(
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createBarrier(*contInsertPoint, llvm::omp::OMPD_for);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   tempTerminator->eraseFromParent();
   builder.restoreIP(*afterIP);
@@ -1044,8 +1089,8 @@ convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
           ompLoc, allocaIP, sectionCBs, privCB, finiCB, false,
           sectionsOp.getNowait());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
 
@@ -1091,8 +1136,8 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
           ompLoc, bodyCB, finiCB, singleOp.getNowait(), llvmCPVars,
           llvmCPFuncs);
 
-  if (!afterIP)
-    return singleOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *singleOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1137,8 +1182,8 @@ convertOmpTeams(omp::TeamsOp op, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTeams(
           ompLoc, bodyCB, numTeamsLower, numTeamsUpper, threadLimit, ifExpr);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1206,8 +1251,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
           moduleTranslation.lookupValue(taskOp.getFinal()),
           moduleTranslation.lookupValue(taskOp.getIfExpr()), dds);
 
-  if (!afterIP)
-    return taskOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *taskOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1234,8 +1279,8 @@ convertOmpTaskgroupOp(omp::TaskgroupOp tgOp, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTaskgroup(ompLoc, allocaIP,
                                                             bodyCB);
 
-  if (!afterIP)
-    return tgOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *tgOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1358,15 +1403,15 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, loopOp.getLoopInclusive(), computeIP);
 
-    if (!result)
-      return loopOp.emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops. Store the insertion point because LoopInfos may get
@@ -1389,8 +1434,8 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
           scheduleMod == omp::ScheduleModifier::monotonic,
           scheduleMod == omp::ScheduleModifier::nonmonotonic, isOrdered);
 
-  if (!wsloopIP)
-    return opInst.emitError(llvm::toString(wsloopIP.takeError()));
+  if (failed(handleError(wsloopIP, opInst)))
+    return failure();
 
   // Continue building IR after the loop. Note that the LoopInfo returned by
   // `collapseLoops` points inside the outermost loop and is intended for
@@ -1672,7 +1717,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
         return contInsertPoint.takeError();
 
       if (!contInsertPoint->getBlock())
-        return llvm::createStringError("failed to convert reductions");
+        return llvm::make_error<SilentTranslationError>();
 
       tempTerminator->eraseFromParent();
       builder.restoreIP(*contInsertPoint);
@@ -1743,8 +1788,9 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createParallel(ompLoc, allocaIP, bodyGenCB, privCB, finiCB,
                                  ifCond, numThreads, pbKind, isCancellable);
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1836,15 +1882,15 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, /*InclusiveStop=*/true, computeIP);
 
-    if (!result)
-      return loopOp->emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops.
@@ -2003,8 +2049,7 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
     moduleTranslation.mapValue(*opInst.getRegion().args_begin(), atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2021,8 +2066,8 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
                                      atomicOrdering, binop, updateFn,
                                      isXBinopExpr);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -2096,8 +2141,7 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
                                atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2114,8 +2158,8 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
           ompLoc, allocaIP, llvmAtomicX, llvmAtomicV, llvmExpr, atomicOrdering,
           binop, updateFn, atomicUpdateOp, isPostfixUpdate, isXBinopExpr);
 
-  if (!afterIP)
-    return atomicCaptureOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *atomicCaptureOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3132,8 +3176,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     case BodyGenTy::DupNoPriv:
@@ -3155,8 +3198,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     }
@@ -3176,8 +3218,8 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
                                         info, genMapInfoCB, &RTLFn);
   }();
 
-  if (!afterIP)
-    return op->emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3581,16 +3623,16 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
   buildDependData(targetOp.getDependKinds(), targetOp.getDependVars(),
                   moduleTranslation, dds);
 
-  llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+  llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       moduleTranslation.getOpenMPBuilder()->createTarget(
           ompLoc, isOffloadEntry, allocaIP, builder.saveIP(), entryInfo,
           defaultValTeams, defaultValThreads, kernelInput, genMapInfoCB, bodyCB,
           argAccessorCB, dds, targetOp.getNowait());
 
-  if (!result)
-    return opInst.emitError(llvm::toString(result.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
-  builder.restoreIP(*result);
+  builder.restoreIP(*afterIP);
 
   // Remap access operations to declare target reference pointers for the
   // device, essentially generating extra loadop's as necessary
@@ -3720,13 +3762,10 @@ convertHostOrTargetOperation(Operation *op, llvm::IRBuilderBase &builder,
 
   return llvm::TypeSwitch<Operation *, LogicalResult>(op)
       .Case([&](omp::BarrierOp) -> LogicalResult {
-        llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+        llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
             ompBuilder->createBarrier(builder.saveIP(),
                                       llvm::omp::OMPD_barrier);
-        if (!result)
-          return op->emitError(llvm::toString(result.takeError()));
-
-        return success();
+        return handleError(afterIP, *op);
       })
       .Case([&](omp::TaskyieldOp) {
         ompBuilder->createTaskyield(builder.saveIP());
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
new file mode 100644
index 00000000000000..f09c9e5785d885
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -0,0 +1,512 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file -verify-diagnostics %s
+
+llvm.func @cancel() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancel}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancel}}
+    omp.cancel cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @cancellation_point() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancellation_point}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancellation_point}}
+    omp.cancellation_point cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @distribute(%lb : i32, %ub : i32, %step : i32) {
+  // expected-error@below {{unsupported OpenMP operation: omp.distribute}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.distribute}}
+  omp.distribute {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @ordered_region_par_level_simd() {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.ordered.region}}
+  omp.ordered.region par_level_simd {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @sections_allocate(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections allocate(%x : !llvm.ptr -> %x : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @sections_private(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections private(@x.privatizer %x -> %arg0 : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @simd_linear(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{linear clause not yet supported}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.simd}}
+  omp.simd linear(%x = %step : !llvm.ptr) {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @simd_private(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{privatization clauses not yet supported}}
+  // expected-error@below {{LLVM Translat...
[truncated]

llvmbot · 2024-10-29T11:48:54Z

@llvm/pr-subscribers-mlir-openmp

Author: Sergio Afonso (skatrak)

Changes

This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom ErrorInfo subclass.

Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

Patch is 31.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114036.diff

3 Files Affected:

(modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+1-7)
(modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (+102-63)
(added) mlir/test/Target/LLVMIR/openmp-todo.mlir (+512)

diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index e1df647d6a3c71..4a27a5ed8eb74b 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2499,13 +2499,7 @@ void OrderedRegionOp::build(OpBuilder &builder, OperationState &state,
   OrderedRegionOp::build(builder, state, clauses.parLevelSimd);
 }
 
-LogicalResult OrderedRegionOp::verify() {
-  // TODO: The code generation for ordered simd directive is not supported yet.
-  if (getParLevelSimd())
-    return failure();
-
-  return verifyOrderedParent(**this);
-}
+LogicalResult OrderedRegionOp::verify() { return verifyOrderedParent(**this); }
 
 //===----------------------------------------------------------------------===//
 // TaskwaitOp
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index fc2f88b766f1c5..1df7b3c083a3a7 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -89,8 +89,54 @@ class OpenMPVarMappingStackFrame
 
   DenseMap<Value, llvm::Value *> mapping;
 };
+
+/// Custom error class to signal translation errors that don't need reporting,
+/// since encountering them will have already triggered relevant error messages.
+///
+/// For example, it should be used to trigger errors from within callbacks
+/// passed to the \see OpenMPIRBuilder when these errors resulted from the
+/// translation of their own regions. This unclutters the error log from
+/// redundant messages.
+class SilentTranslationError : public llvm::ErrorInfo<SilentTranslationError> {
+public:
+  void log(raw_ostream &) const override {
+    // Do not log anything.
+  }
+
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable(
+        "SilentTranslationError doesn't support ECError conversion");
+  }
+
+  // Used by ErrorInfo::classID.
+  static char ID;
+};
+
+char SilentTranslationError::ID = 0;
+
 } // namespace
 
+static LogicalResult handleError(llvm::Error error, Operation &op) {
+  LogicalResult result = success();
+  if (error) {
+    llvm::handleAllErrors(
+        std::move(error),
+        [&](const SilentTranslationError &) { result = failure(); },
+        [&](const llvm::ErrorInfoBase &err) {
+          result = op.emitError(err.message());
+        });
+  }
+  return result;
+}
+
+template <typename T>
+static LogicalResult handleError(llvm::Expected<T> &result, Operation &op) {
+  if (!result)
+    return handleError(result.takeError(), op);
+
+  return success();
+}
+
 /// Find the insertion point for allocas given the current insertion point for
 /// normal operations in the builder.
 static llvm::OpenMPIRBuilder::InsertPointTy
@@ -216,7 +262,7 @@ static llvm::Expected<llvm::BasicBlock *> convertOmpOpRegions(
     llvm::IRBuilderBase::InsertPointGuard guard(builder);
     if (failed(
             moduleTranslation.convertBlock(*bb, bb->isEntryBlock(), builder)))
-      return llvm::createStringError("failed region translation");
+      return llvm::make_error<SilentTranslationError>();
 
     // Special handling for `omp.yield` and `omp.terminator` (we may have more
     // than one): they return the control to the parent OpenMP dialect operation
@@ -296,8 +342,8 @@ convertOmpMasked(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMasked(ompLoc, bodyGenCB,
                                                          finiCB, filterVal);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -326,8 +372,8 @@ convertOmpMaster(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMaster(ompLoc, bodyGenCB,
                                                          finiCB);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -373,8 +419,8 @@ convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createCritical(
           ompLoc, bodyGenCB, finiCB, criticalOp.getName().value_or(""), hint);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -481,9 +527,8 @@ static LogicalResult inlineConvertOmpRegions(
   llvm::Expected<llvm::BasicBlock *> continuationBlock =
       convertOmpOpRegions(region, blockName, builder, moduleTranslation, &phis);
 
-  if (!continuationBlock)
-    return region.getParentOp()->emitError(
-        llvm::toString(continuationBlock.takeError()));
+  if (failed(handleError(continuationBlock, *region.getParentOp())))
+    return failure();
 
   if (continuationBlockArgs)
     llvm::append_range(*continuationBlockArgs, phis);
@@ -605,7 +650,7 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
 
   // TODO: The code generation for ordered simd directive is not supported yet.
   if (orderedRegionOp.getParLevelSimd())
-    return failure();
+    return opInst.emitError("unhandled clauses for translation to LLVM IR");
 
   auto bodyGenCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
     // OrderedOp has only one region associated with it.
@@ -625,8 +670,8 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createOrderedThreadsSimd(
           ompLoc, bodyGenCB, finiCB, !orderedRegionOp.getParLevelSimd());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -831,8 +876,8 @@ static LogicalResult createReductionsAndCleanup(
       ompBuilder->createReductions(builder.saveIP(), allocaIP, reductionInfos,
                                    isByRef, op.getNowait());
 
-  if (!contInsertPoint)
-    return op.emitError(llvm::toString(contInsertPoint.takeError()));
+  if (failed(handleError(contInsertPoint, *op)))
+    return failure();
 
   if (!contInsertPoint->getBlock())
     return op->emitOpError() << "failed to convert reductions";
@@ -840,8 +885,8 @@ static LogicalResult createReductionsAndCleanup(
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createBarrier(*contInsertPoint, llvm::omp::OMPD_for);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   tempTerminator->eraseFromParent();
   builder.restoreIP(*afterIP);
@@ -1044,8 +1089,8 @@ convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
           ompLoc, allocaIP, sectionCBs, privCB, finiCB, false,
           sectionsOp.getNowait());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
 
@@ -1091,8 +1136,8 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
           ompLoc, bodyCB, finiCB, singleOp.getNowait(), llvmCPVars,
           llvmCPFuncs);
 
-  if (!afterIP)
-    return singleOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *singleOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1137,8 +1182,8 @@ convertOmpTeams(omp::TeamsOp op, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTeams(
           ompLoc, bodyCB, numTeamsLower, numTeamsUpper, threadLimit, ifExpr);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1206,8 +1251,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
           moduleTranslation.lookupValue(taskOp.getFinal()),
           moduleTranslation.lookupValue(taskOp.getIfExpr()), dds);
 
-  if (!afterIP)
-    return taskOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *taskOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1234,8 +1279,8 @@ convertOmpTaskgroupOp(omp::TaskgroupOp tgOp, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTaskgroup(ompLoc, allocaIP,
                                                             bodyCB);
 
-  if (!afterIP)
-    return tgOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *tgOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1358,15 +1403,15 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, loopOp.getLoopInclusive(), computeIP);
 
-    if (!result)
-      return loopOp.emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops. Store the insertion point because LoopInfos may get
@@ -1389,8 +1434,8 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
           scheduleMod == omp::ScheduleModifier::monotonic,
           scheduleMod == omp::ScheduleModifier::nonmonotonic, isOrdered);
 
-  if (!wsloopIP)
-    return opInst.emitError(llvm::toString(wsloopIP.takeError()));
+  if (failed(handleError(wsloopIP, opInst)))
+    return failure();
 
   // Continue building IR after the loop. Note that the LoopInfo returned by
   // `collapseLoops` points inside the outermost loop and is intended for
@@ -1672,7 +1717,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
         return contInsertPoint.takeError();
 
       if (!contInsertPoint->getBlock())
-        return llvm::createStringError("failed to convert reductions");
+        return llvm::make_error<SilentTranslationError>();
 
       tempTerminator->eraseFromParent();
       builder.restoreIP(*contInsertPoint);
@@ -1743,8 +1788,9 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createParallel(ompLoc, allocaIP, bodyGenCB, privCB, finiCB,
                                  ifCond, numThreads, pbKind, isCancellable);
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1836,15 +1882,15 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, /*InclusiveStop=*/true, computeIP);
 
-    if (!result)
-      return loopOp->emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops.
@@ -2003,8 +2049,7 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
     moduleTranslation.mapValue(*opInst.getRegion().args_begin(), atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2021,8 +2066,8 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
                                      atomicOrdering, binop, updateFn,
                                      isXBinopExpr);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -2096,8 +2141,7 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
                                atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2114,8 +2158,8 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
           ompLoc, allocaIP, llvmAtomicX, llvmAtomicV, llvmExpr, atomicOrdering,
           binop, updateFn, atomicUpdateOp, isPostfixUpdate, isXBinopExpr);
 
-  if (!afterIP)
-    return atomicCaptureOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *atomicCaptureOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3132,8 +3176,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     case BodyGenTy::DupNoPriv:
@@ -3155,8 +3198,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     }
@@ -3176,8 +3218,8 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
                                         info, genMapInfoCB, &RTLFn);
   }();
 
-  if (!afterIP)
-    return op->emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3581,16 +3623,16 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
   buildDependData(targetOp.getDependKinds(), targetOp.getDependVars(),
                   moduleTranslation, dds);
 
-  llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+  llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       moduleTranslation.getOpenMPBuilder()->createTarget(
           ompLoc, isOffloadEntry, allocaIP, builder.saveIP(), entryInfo,
           defaultValTeams, defaultValThreads, kernelInput, genMapInfoCB, bodyCB,
           argAccessorCB, dds, targetOp.getNowait());
 
-  if (!result)
-    return opInst.emitError(llvm::toString(result.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
-  builder.restoreIP(*result);
+  builder.restoreIP(*afterIP);
 
   // Remap access operations to declare target reference pointers for the
   // device, essentially generating extra loadop's as necessary
@@ -3720,13 +3762,10 @@ convertHostOrTargetOperation(Operation *op, llvm::IRBuilderBase &builder,
 
   return llvm::TypeSwitch<Operation *, LogicalResult>(op)
       .Case([&](omp::BarrierOp) -> LogicalResult {
-        llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+        llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
             ompBuilder->createBarrier(builder.saveIP(),
                                       llvm::omp::OMPD_barrier);
-        if (!result)
-          return op->emitError(llvm::toString(result.takeError()));
-
-        return success();
+        return handleError(afterIP, *op);
       })
       .Case([&](omp::TaskyieldOp) {
         ompBuilder->createTaskyield(builder.saveIP());
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
new file mode 100644
index 00000000000000..f09c9e5785d885
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -0,0 +1,512 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file -verify-diagnostics %s
+
+llvm.func @cancel() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancel}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancel}}
+    omp.cancel cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @cancellation_point() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancellation_point}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancellation_point}}
+    omp.cancellation_point cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @distribute(%lb : i32, %ub : i32, %step : i32) {
+  // expected-error@below {{unsupported OpenMP operation: omp.distribute}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.distribute}}
+  omp.distribute {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @ordered_region_par_level_simd() {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.ordered.region}}
+  omp.ordered.region par_level_simd {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @sections_allocate(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections allocate(%x : !llvm.ptr -> %x : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @sections_private(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections private(@x.privatizer %x -> %arg0 : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @simd_linear(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{linear clause not yet supported}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.simd}}
+  omp.simd linear(%x = %step : !llvm.ptr) {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @simd_private(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{privatization clauses not yet supported}}
+  // expected-error@below {{LLVM Translat...
[truncated]

llvmbot · 2024-10-29T11:48:54Z

@llvm/pr-subscribers-flang-openmp

Author: Sergio Afonso (skatrak)

Changes

This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom ErrorInfo subclass.

Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

Patch is 31.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114036.diff

3 Files Affected:

(modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+1-7)
(modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (+102-63)
(added) mlir/test/Target/LLVMIR/openmp-todo.mlir (+512)

diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index e1df647d6a3c71..4a27a5ed8eb74b 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2499,13 +2499,7 @@ void OrderedRegionOp::build(OpBuilder &builder, OperationState &state,
   OrderedRegionOp::build(builder, state, clauses.parLevelSimd);
 }
 
-LogicalResult OrderedRegionOp::verify() {
-  // TODO: The code generation for ordered simd directive is not supported yet.
-  if (getParLevelSimd())
-    return failure();
-
-  return verifyOrderedParent(**this);
-}
+LogicalResult OrderedRegionOp::verify() { return verifyOrderedParent(**this); }
 
 //===----------------------------------------------------------------------===//
 // TaskwaitOp
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index fc2f88b766f1c5..1df7b3c083a3a7 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -89,8 +89,54 @@ class OpenMPVarMappingStackFrame
 
   DenseMap<Value, llvm::Value *> mapping;
 };
+
+/// Custom error class to signal translation errors that don't need reporting,
+/// since encountering them will have already triggered relevant error messages.
+///
+/// For example, it should be used to trigger errors from within callbacks
+/// passed to the \see OpenMPIRBuilder when these errors resulted from the
+/// translation of their own regions. This unclutters the error log from
+/// redundant messages.
+class SilentTranslationError : public llvm::ErrorInfo<SilentTranslationError> {
+public:
+  void log(raw_ostream &) const override {
+    // Do not log anything.
+  }
+
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable(
+        "SilentTranslationError doesn't support ECError conversion");
+  }
+
+  // Used by ErrorInfo::classID.
+  static char ID;
+};
+
+char SilentTranslationError::ID = 0;
+
 } // namespace
 
+static LogicalResult handleError(llvm::Error error, Operation &op) {
+  LogicalResult result = success();
+  if (error) {
+    llvm::handleAllErrors(
+        std::move(error),
+        [&](const SilentTranslationError &) { result = failure(); },
+        [&](const llvm::ErrorInfoBase &err) {
+          result = op.emitError(err.message());
+        });
+  }
+  return result;
+}
+
+template <typename T>
+static LogicalResult handleError(llvm::Expected<T> &result, Operation &op) {
+  if (!result)
+    return handleError(result.takeError(), op);
+
+  return success();
+}
+
 /// Find the insertion point for allocas given the current insertion point for
 /// normal operations in the builder.
 static llvm::OpenMPIRBuilder::InsertPointTy
@@ -216,7 +262,7 @@ static llvm::Expected<llvm::BasicBlock *> convertOmpOpRegions(
     llvm::IRBuilderBase::InsertPointGuard guard(builder);
     if (failed(
             moduleTranslation.convertBlock(*bb, bb->isEntryBlock(), builder)))
-      return llvm::createStringError("failed region translation");
+      return llvm::make_error<SilentTranslationError>();
 
     // Special handling for `omp.yield` and `omp.terminator` (we may have more
     // than one): they return the control to the parent OpenMP dialect operation
@@ -296,8 +342,8 @@ convertOmpMasked(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMasked(ompLoc, bodyGenCB,
                                                          finiCB, filterVal);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -326,8 +372,8 @@ convertOmpMaster(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createMaster(ompLoc, bodyGenCB,
                                                          finiCB);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -373,8 +419,8 @@ convertOmpCritical(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createCritical(
           ompLoc, bodyGenCB, finiCB, criticalOp.getName().value_or(""), hint);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -481,9 +527,8 @@ static LogicalResult inlineConvertOmpRegions(
   llvm::Expected<llvm::BasicBlock *> continuationBlock =
       convertOmpOpRegions(region, blockName, builder, moduleTranslation, &phis);
 
-  if (!continuationBlock)
-    return region.getParentOp()->emitError(
-        llvm::toString(continuationBlock.takeError()));
+  if (failed(handleError(continuationBlock, *region.getParentOp())))
+    return failure();
 
   if (continuationBlockArgs)
     llvm::append_range(*continuationBlockArgs, phis);
@@ -605,7 +650,7 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
 
   // TODO: The code generation for ordered simd directive is not supported yet.
   if (orderedRegionOp.getParLevelSimd())
-    return failure();
+    return opInst.emitError("unhandled clauses for translation to LLVM IR");
 
   auto bodyGenCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
     // OrderedOp has only one region associated with it.
@@ -625,8 +670,8 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createOrderedThreadsSimd(
           ompLoc, bodyGenCB, finiCB, !orderedRegionOp.getParLevelSimd());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -831,8 +876,8 @@ static LogicalResult createReductionsAndCleanup(
       ompBuilder->createReductions(builder.saveIP(), allocaIP, reductionInfos,
                                    isByRef, op.getNowait());
 
-  if (!contInsertPoint)
-    return op.emitError(llvm::toString(contInsertPoint.takeError()));
+  if (failed(handleError(contInsertPoint, *op)))
+    return failure();
 
   if (!contInsertPoint->getBlock())
     return op->emitOpError() << "failed to convert reductions";
@@ -840,8 +885,8 @@ static LogicalResult createReductionsAndCleanup(
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createBarrier(*contInsertPoint, llvm::omp::OMPD_for);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   tempTerminator->eraseFromParent();
   builder.restoreIP(*afterIP);
@@ -1044,8 +1089,8 @@ convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
           ompLoc, allocaIP, sectionCBs, privCB, finiCB, false,
           sectionsOp.getNowait());
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
 
@@ -1091,8 +1136,8 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
           ompLoc, bodyCB, finiCB, singleOp.getNowait(), llvmCPVars,
           llvmCPFuncs);
 
-  if (!afterIP)
-    return singleOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *singleOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1137,8 +1182,8 @@ convertOmpTeams(omp::TeamsOp op, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTeams(
           ompLoc, bodyCB, numTeamsLower, numTeamsUpper, threadLimit, ifExpr);
 
-  if (!afterIP)
-    return op.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1206,8 +1251,8 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
           moduleTranslation.lookupValue(taskOp.getFinal()),
           moduleTranslation.lookupValue(taskOp.getIfExpr()), dds);
 
-  if (!afterIP)
-    return taskOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *taskOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1234,8 +1279,8 @@ convertOmpTaskgroupOp(omp::TaskgroupOp tgOp, llvm::IRBuilderBase &builder,
       moduleTranslation.getOpenMPBuilder()->createTaskgroup(ompLoc, allocaIP,
                                                             bodyCB);
 
-  if (!afterIP)
-    return tgOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *tgOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1358,15 +1403,15 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, loopOp.getLoopInclusive(), computeIP);
 
-    if (!result)
-      return loopOp.emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops. Store the insertion point because LoopInfos may get
@@ -1389,8 +1434,8 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
           scheduleMod == omp::ScheduleModifier::monotonic,
           scheduleMod == omp::ScheduleModifier::nonmonotonic, isOrdered);
 
-  if (!wsloopIP)
-    return opInst.emitError(llvm::toString(wsloopIP.takeError()));
+  if (failed(handleError(wsloopIP, opInst)))
+    return failure();
 
   // Continue building IR after the loop. Note that the LoopInfo returned by
   // `collapseLoops` points inside the outermost loop and is intended for
@@ -1672,7 +1717,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
         return contInsertPoint.takeError();
 
       if (!contInsertPoint->getBlock())
-        return llvm::createStringError("failed to convert reductions");
+        return llvm::make_error<SilentTranslationError>();
 
       tempTerminator->eraseFromParent();
       builder.restoreIP(*contInsertPoint);
@@ -1743,8 +1788,9 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
   llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       ompBuilder->createParallel(ompLoc, allocaIP, bodyGenCB, privCB, finiCB,
                                  ifCond, numThreads, pbKind, isCancellable);
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -1836,15 +1882,15 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
       computeIP = loopInfos.front()->getPreheaderIP();
     }
 
-    llvm::Expected<llvm::CanonicalLoopInfo *> result =
+    llvm::Expected<llvm::CanonicalLoopInfo *> loopResult =
         ompBuilder->createCanonicalLoop(
             loc, bodyGen, lowerBound, upperBound, step,
             /*IsSigned=*/true, /*InclusiveStop=*/true, computeIP);
 
-    if (!result)
-      return loopOp->emitError(llvm::toString(result.takeError()));
+    if (failed(handleError(loopResult, *loopOp)))
+      return failure();
 
-    loopInfos.push_back(*result);
+    loopInfos.push_back(*loopResult);
   }
 
   // Collapse loops.
@@ -2003,8 +2049,7 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
     moduleTranslation.mapValue(*opInst.getRegion().args_begin(), atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2021,8 +2066,8 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
                                      atomicOrdering, binop, updateFn,
                                      isXBinopExpr);
 
-  if (!afterIP)
-    return opInst.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *opInst)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -2096,8 +2141,7 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
                                atomicx);
     moduleTranslation.mapBlock(&bb, builder.GetInsertBlock());
     if (failed(moduleTranslation.convertBlock(bb, true, builder)))
-      return llvm::createStringError(
-          "unable to convert update operation to llvm IR");
+      return llvm::make_error<SilentTranslationError>();
 
     omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
     assert(yieldop && yieldop.getResults().size() == 1 &&
@@ -2114,8 +2158,8 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
           ompLoc, allocaIP, llvmAtomicX, llvmAtomicV, llvmExpr, atomicOrdering,
           binop, updateFn, atomicUpdateOp, isPostfixUpdate, isXBinopExpr);
 
-  if (!afterIP)
-    return atomicCaptureOp.emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *atomicCaptureOp)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3132,8 +3176,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     case BodyGenTy::DupNoPriv:
@@ -3155,8 +3198,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
 
         if (failed(inlineConvertOmpRegions(region, "omp.data.region", builder,
                                            moduleTranslation)))
-          return llvm::createStringError(
-              "failed to inline region of an `omp.target_data` op");
+          return llvm::make_error<SilentTranslationError>();
       }
       break;
     }
@@ -3176,8 +3218,8 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
                                         info, genMapInfoCB, &RTLFn);
   }();
 
-  if (!afterIP)
-    return op->emitError(llvm::toString(afterIP.takeError()));
+  if (failed(handleError(afterIP, *op)))
+    return failure();
 
   builder.restoreIP(*afterIP);
   return success();
@@ -3581,16 +3623,16 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
   buildDependData(targetOp.getDependKinds(), targetOp.getDependVars(),
                   moduleTranslation, dds);
 
-  llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+  llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
       moduleTranslation.getOpenMPBuilder()->createTarget(
           ompLoc, isOffloadEntry, allocaIP, builder.saveIP(), entryInfo,
           defaultValTeams, defaultValThreads, kernelInput, genMapInfoCB, bodyCB,
           argAccessorCB, dds, targetOp.getNowait());
 
-  if (!result)
-    return opInst.emitError(llvm::toString(result.takeError()));
+  if (failed(handleError(afterIP, opInst)))
+    return failure();
 
-  builder.restoreIP(*result);
+  builder.restoreIP(*afterIP);
 
   // Remap access operations to declare target reference pointers for the
   // device, essentially generating extra loadop's as necessary
@@ -3720,13 +3762,10 @@ convertHostOrTargetOperation(Operation *op, llvm::IRBuilderBase &builder,
 
   return llvm::TypeSwitch<Operation *, LogicalResult>(op)
       .Case([&](omp::BarrierOp) -> LogicalResult {
-        llvm::OpenMPIRBuilder::InsertPointOrErrorTy result =
+        llvm::OpenMPIRBuilder::InsertPointOrErrorTy afterIP =
             ompBuilder->createBarrier(builder.saveIP(),
                                       llvm::omp::OMPD_barrier);
-        if (!result)
-          return op->emitError(llvm::toString(result.takeError()));
-
-        return success();
+        return handleError(afterIP, *op);
       })
       .Case([&](omp::TaskyieldOp) {
         ompBuilder->createTaskyield(builder.saveIP());
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
new file mode 100644
index 00000000000000..f09c9e5785d885
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -0,0 +1,512 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file -verify-diagnostics %s
+
+llvm.func @cancel() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancel}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancel}}
+    omp.cancel cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @cancellation_point() {
+  // expected-error@below {{LLVM Translation failed for operation: omp.parallel}}
+  omp.parallel {
+    // expected-error@below {{unsupported OpenMP operation: omp.cancellation_point}}
+    // expected-error@below {{LLVM Translation failed for operation: omp.cancellation_point}}
+    omp.cancellation_point cancellation_construct_type(parallel)
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @distribute(%lb : i32, %ub : i32, %step : i32) {
+  // expected-error@below {{unsupported OpenMP operation: omp.distribute}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.distribute}}
+  omp.distribute {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @ordered_region_par_level_simd() {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.ordered.region}}
+  omp.ordered.region par_level_simd {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @sections_allocate(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections allocate(%x : !llvm.ptr -> %x : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @sections_private(%x : !llvm.ptr) {
+  // expected-error@below {{unhandled clauses for translation to LLVM IR}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.sections}}
+  omp.sections private(@x.privatizer %x -> %arg0 : !llvm.ptr) {
+    omp.terminator
+  }
+  llvm.return
+}
+
+// -----
+
+llvm.func @simd_linear(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{linear clause not yet supported}}
+  // expected-error@below {{LLVM Translation failed for operation: omp.simd}}
+  omp.simd linear(%x = %step : !llvm.ptr) {
+    omp.loop_nest (%iv) : i32 = (%lb) to (%ub) step (%step) {
+      omp.yield
+    }
+  }
+  llvm.return
+}
+
+// -----
+
+omp.private {type = private} @x.privatizer : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  %1 = llvm.alloca %0 x i32 : (i32) -> !llvm.ptr
+  omp.yield(%1 : !llvm.ptr)
+}
+llvm.func @simd_private(%lb : i32, %ub : i32, %step : i32, %x : !llvm.ptr) {
+  // expected-error@below {{privatization clauses not yet supported}}
+  // expected-error@below {{LLVM Translat...
[truncated]

skatrak · 2024-10-29T11:48:57Z

PR stack:

Meinersbur

Is the following understanding correct? If so, can you add such explanations to the doxygen?

This is intended to be the glue between MLIR error API using llvm::LogicalResult that is just a flag whether an error occured and LLVM using llvm::Error/llvm::Expected that additionally also carries the error message itself. Since LogicialResult does not contain the error, that API must have already reported the error but still need to pass something as llvm::Error, respectively must report the error when converting it to LogicalResult.

Meinersbur · 2024-10-29T15:07:57Z

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

+/// passed to the \see OpenMPIRBuilder when these errors resulted from the
+/// translation of their own regions. This unclutters the error log from
+/// redundant messages.
+class SilentTranslationError : public llvm::ErrorInfo<SilentTranslationError> {


Could we think of a better name? These should not be ignored, just that is is expected to already have been reported.

I think I improved it a bit by naming it PreviouslyReportedError, but I don't know if others will find that to be a good name. Let me know if you have a better alternative.

skatrak · 2024-10-29T16:24:48Z

Is the following understanding correct? If so, can you add such explanations to the doxygen?

This is intended to be the glue between MLIR error API using llvm::LogicalResult that is just a flag whether an error occured and LLVM using llvm::Error/llvm::Expected that additionally also carries the error message itself. Since LogicialResult does not contain the error, that API must have already reported the error but still need to pass something as llvm::Error, respectively must report the error when converting it to LogicalResult.

Yes, that's a pretty good explanation of what this is for. I updated the class description to hopefully capture the need for it a bit better.

Meinersbur

LGTM

tblah

Thanks!

…14036) This patch unifies the handling of errors passed through the OpenMPIRBuilder and removes some redundant error messages through the introduction of a custom `ErrorInfo` subclass. Additionally, the current list of operations and clauses unsupported by the MLIR to LLVM IR translation pass is added to a new Lit test to check they are being reported to the user.

skatrak requested review from ergawy, Meinersbur, jsjodin, tblah, kiranchandramohan, abidh, agozillon, DominikAdamski, TIFitis, kparzysz, anchuraj and bhandarkar-pranav October 29, 2024 11:48

llvmbot added mlir:llvm mlir mlir:openmp flang:openmp labels Oct 29, 2024

skatrak mentioned this pull request Oct 29, 2024

[MLIR][OpenMP] Emit descriptive errors for all unsupported clauses #114037

Merged

Meinersbur reviewed Oct 29, 2024

View reviewed changes

Address review comments

ed14ee8

Meinersbur approved these changes Oct 29, 2024

View reviewed changes

tblah approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'main' into users/skatrak/mlir-omp-err-01

12c3878

skatrak merged commit 21a6032 into main Oct 31, 2024
6 of 7 checks passed

skatrak deleted the users/skatrak/mlir-omp-err-01 branch October 31, 2024 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036

[MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036

Uh oh!

skatrak commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

skatrak commented Oct 29, 2024

Uh oh!

Meinersbur left a comment

Uh oh!

Meinersbur Oct 29, 2024

Uh oh!

skatrak Oct 29, 2024

Uh oh!

skatrak commented Oct 29, 2024

Uh oh!

Meinersbur left a comment

Uh oh!

tblah left a comment

Uh oh!

Uh oh!

Uh oh!

[MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036

[MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036

Uh oh!

Conversation

skatrak commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

skatrak commented Oct 29, 2024

Uh oh!

Meinersbur left a comment

Choose a reason for hiding this comment

Uh oh!

Meinersbur Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

skatrak Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

skatrak commented Oct 29, 2024

Uh oh!

Meinersbur left a comment

Choose a reason for hiding this comment

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Oct 29, 2024 •

edited

Loading