[Mlir-commits] [mlir] [MLIR][OpenMP] Simplify translation to LLVM IR error handling (PR #114036)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Tue Oct 29 04:48:59 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir
Author: Sergio Afonso (skatrak)
<details>
<summary>Changes</summary>
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
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 at below {{LLVM Translation failed for operation: omp.parallel}}
+ omp.parallel {
+ // expected-error at below {{unsupported OpenMP operation: omp.cancel}}
+ // expected-error at below {{LLVM Translation failed for operation: omp.cancel}}
+ omp.cancel cancellation_construct_type(parallel)
+ omp.terminator
+ }
+ llvm.return
+}
+
+// -----
+
+llvm.func @cancellation_point() {
+ // expected-error at below {{LLVM Translation failed for operation: omp.parallel}}
+ omp.parallel {
+ // expected-error at below {{unsupported OpenMP operation: omp.cancellation_point}}
+ // expected-error at 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 at below {{unsupported OpenMP operation: omp.distribute}}
+ // expected-error at 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 at below {{unhandled clauses for translation to LLVM IR}}
+ // expected-error at 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 at below {{unhandled clauses for translation to LLVM IR}}
+ // expected-error at 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 at below {{unhandled clauses for translation to LLVM IR}}
+ // expected-error at 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 at below {{linear clause not yet supported}}
+ // expected-error at 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 at below {{privatization clauses not yet supported}}
+ // expected-error at below {{LLVM Translat...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/114036
More information about the Mlir-commits
mailing list