[OpenMP][OMPIRBuilder] Error propagation across callbacks #112533
Conversation
skatrak
requested review from
ergawy,
jsjodin,
tblah,
kiranchandramohan,
agozillon,
DominikAdamski,
TIFitis,
kparzysz and
bhandarkar-pranav
skatrak
force-pushed
the
omp-mlir-llvm-error-handling
branch
from
82d2acd
to
201a1fa
Compare
|
My naive view would be that once we start emitting code, other than non-recoverable errors such as bugs and non-yet-implemented aborts where the compiler should just crash. For descriptive messages for crashes, there is This is what adds to Clang crash reports. "failed region translation" (or more descriptive: "translating region 'X'") could be an entry added in OpenMPIRBuilder. In other words, what recoverable error will this be used for? |
|
Thanks Michael for sharing your thoughts. I agree with you, in that I also think these errors are unrecoverable. The problem at the moment is that the OMPIRBuilder can't check whether the callbacks it calls have failed, so its codegen functions continue execution assuming the current state is valid. So, currently, when they fail for any of the reasons you mentioned (bugs, unsupported ops, etc) it can then dereference null pointers, trigger asserts later on, etc (see #109755 for an example). So one solution to this would be what I understand you're proposing, which would be to print a crash report and exit the program directly from within these callbacks, so they never return. However, if you look everywhere else in the omp dialect to LLVM IR translation, you'll see that the expected method to handle errors like these there is to emit MLIR diagnostics and then return early with a The solution I'm proposing is addressing long-standing TODO comments for supporting "error propagation in OpenMPIRBuilder" by:
|
|
I think continuing on an unrecoverable error is an inherently bad idea. It will create code paths that will never be tested by definition (because if there is a path we will fix the bug leading to that path) and hence likely cause dereferenced null pointers, trigger assertions, etc already because the LLVM generated so far is not in an expected state. For instance, the new early abort in However, of course I will not stand in the way if this is the community-chosen solution and/or established pattern. Also, if there is use case with a recoverable error we would need this as well. |
|
I don't feel strongly about this, but I think the proposed solution looks like a step forward. I think it is important that we do error out as soon as possible after an unrecoverable error and I think this is a cleaner way to do that than setting captured variables like I think the value in having some controlled error reporting like this over an assertion is for cases where implementation is still in progress. This allows us to print something more user friendly than a segfault/stack trace. As a reviewer I would read something like these errors as "this is a known limitation or error condition" whereas an assertion as "this is assumed never to happen". |
|
I might have overblown the problem. If with every MLIR uses Rust-style error propagation, and since OpenMPIRBuilder is used within MLIR, it should be compatible with it. I am still not convinced that it provides any benefit, but consider my objections retracted. |
I agree that once one of these errors were triggered, we'd have to stop any further translation or codegen process. But the approach I'm proposing actually does this. There is indeed some level of collaboration needed at each stage for that to happen, but basically the pattern is already to return
In a way, the changes to the OMPIRBuilder are not really mandatory to make use of. Callbacks passed to it from Clang can decide to exit the program on irrecoverable failure and always return success, and assume the returned |
skatrak
force-pushed
the
omp-mlir-llvm-error-handling
branch
from
201a1fa
to
24ab62d
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-flang-openmp Author: Sergio Afonso (skatrak) ChangesThis is a small proof of concept showing an approach to communicate errors between MLIR to LLVM IR translation of the OpenMP dialect and the OMPIRBuilder. It only implements the approach for a single case, so it doesn't compile or run, since it's only intended to show how it could look like and discuss it before investing too much effort on a full implementation. The main idea is to use This should prevent encountering any unsupported operations or arguments, or any other unexpected error from resulting in a compiler crash. Instead, a descriptive error message is presented to users. Patch is 222.77 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112533.diff 8 Files Affected:
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 3747b00d4893ad..92f2c6904f39de 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -1192,6 +1192,7 @@ struct PushAndPopStackRAII {
CodeGenFunction::JumpDest Dest =
CGF.getOMPCancelDestination(OMPD_parallel);
CGF.EmitBranchThroughCleanup(Dest);
+ return llvm::Error::success();
};
// TODO: Remove this once we emit parallel regions through the
@@ -2331,8 +2332,10 @@ void CGOpenMPRuntime::emitBarrierCall(CodeGenFunction &CGF, SourceLocation Loc,
auto *OMPRegionInfo =
dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo);
if (CGF.CGM.getLangOpts().OpenMPIRBuilder) {
- CGF.Builder.restoreIP(OMPBuilder.createBarrier(
- CGF.Builder, Kind, ForceSimpleCall, EmitChecks));
+ auto Result = OMPBuilder.createBarrier(CGF.Builder, Kind, ForceSimpleCall,
+ EmitChecks);
+ assert(Result && "unexpected error creating barrier");
+ CGF.Builder.restoreIP(*Result);
return;
}
@@ -5928,8 +5931,10 @@ void CGOpenMPRuntime::emitTargetOutlinedFunctionHelper(
return CGF.GenerateOpenMPCapturedStmtFunction(CS, D.getBeginLoc());
};
- OMPBuilder.emitTargetRegionFunction(EntryInfo, GenerateOutlinedFunction,
- IsOffloadEntry, OutlinedFn, OutlinedFnID);
+ llvm::Error Err = OMPBuilder.emitTargetRegionFunction(
+ EntryInfo, GenerateOutlinedFunction, IsOffloadEntry, OutlinedFn,
+ OutlinedFnID);
+ assert(!Err && "unexpected error creating target region");
if (!OutlinedFn)
return;
@@ -9671,9 +9676,11 @@ static void emitTargetCallKernelLaunch(
NumTargetItems, RTArgs, NumIterations, NumTeams, NumThreads,
DynCGGroupMem, HasNoWait);
- CGF.Builder.restoreIP(OMPRuntime->getOMPBuilder().emitKernelLaunch(
+ auto Result = OMPRuntime->getOMPBuilder().emitKernelLaunch(
CGF.Builder, OutlinedFn, OutlinedFnID, EmitTargetCallFallbackCB, Args,
- DeviceID, RTLoc, AllocaIP));
+ DeviceID, RTLoc, AllocaIP);
+ assert(Result && "unexpected error creating kernel launch");
+ CGF.Builder.restoreIP(*Result);
};
if (RequiresOuterTask)
@@ -10350,9 +10357,11 @@ void CGOpenMPRuntime::emitTargetDataCalls(
InsertPointTy CodeGenIP(CGF.Builder.GetInsertBlock(),
CGF.Builder.GetInsertPoint());
llvm::OpenMPIRBuilder::LocationDescription OmpLoc(CodeGenIP);
- CGF.Builder.restoreIP(OMPBuilder.createTargetData(
+ auto Result = OMPBuilder.createTargetData(
OmpLoc, AllocaIP, CodeGenIP, DeviceID, IfCondVal, Info, GenMapInfoCB,
- /*MapperFunc=*/nullptr, BodyCB, DeviceAddrCB, CustomMapperCB, RTLoc));
+ /*MapperFunc=*/nullptr, BodyCB, DeviceAddrCB, CustomMapperCB, RTLoc);
+ assert(Result && "unexpected error creating target data");
+ CGF.Builder.restoreIP(*Result);
}
void CGOpenMPRuntime::emitTargetDataStandAloneCall(
diff --git a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
index 35ff75416cb776..62b8c5475a4ebe 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
@@ -1753,11 +1753,13 @@ void CGOpenMPRuntimeGPU::emitReduction(
Idx++;
}
- CGF.Builder.restoreIP(OMPBuilder.createReductionsGPU(
+ auto Result = OMPBuilder.createReductionsGPU(
OmpLoc, AllocaIP, CodeGenIP, ReductionInfos, false, TeamsReduction,
DistributeReduction, llvm::OpenMPIRBuilder::ReductionGenCBKind::Clang,
CGF.getTarget().getGridValue(), C.getLangOpts().OpenMPCUDAReductionBufNum,
- RTLoc));
+ RTLoc);
+ assert(Result && "unexpected error creating GPU reductions");
+ CGF.Builder.restoreIP(*Result);
return;
}
diff --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp b/clang/lib/CodeGen/CGStmtOpenMP.cpp
index 71a27d0c6bc1fb..0396f7cef89fc7 100644
--- a/clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1809,6 +1809,7 @@ void CodeGenFunction::EmitOMPParallelDirective(const OMPParallelDirective &S) {
// thus calls destructors etc.
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
// Privatization callback that performs appropriate action for
@@ -1831,15 +1832,18 @@ void CodeGenFunction::EmitOMPParallelDirective(const OMPParallelDirective &S) {
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPOutlinedRegionBody(
*this, ParallelRegionBodyStmt, AllocaIP, CodeGenIP, "parallel");
+ return llvm::Error::success();
};
CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
AllocaInsertPt->getParent(), AllocaInsertPt->getIterator());
- Builder.restoreIP(
+ auto Result =
OMPBuilder.createParallel(Builder, AllocaIP, BodyGenCB, PrivCB, FiniCB,
- IfCond, NumThreads, ProcBind, S.hasCancel()));
+ IfCond, NumThreads, ProcBind, S.hasCancel());
+ assert(Result && "unexpected error creating parallel");
+ Builder.restoreIP(*Result);
return;
}
@@ -2128,9 +2132,12 @@ void CodeGenFunction::EmitOMPCanonicalLoop(const OMPCanonicalLoop *S) {
RunCleanupsScope BodyScope(*this);
EmitStmt(BodyStmt);
+ return llvm::Error::success();
};
- llvm::CanonicalLoopInfo *CL =
- OMPBuilder.createCanonicalLoop(Builder, BodyGen, DistVal);
+
+ auto Result = OMPBuilder.createCanonicalLoop(Builder, BodyGen, DistVal);
+ assert(Result && "unexpected error creating parallel");
+ llvm::CanonicalLoopInfo *CL = *Result;
// Finish up the loop.
Builder.restoreIP(CL->getAfterIP());
@@ -4016,11 +4023,12 @@ static void emitOMPForDirective(const OMPLoopDirective &S, CodeGenFunction &CGF,
CGM.getOpenMPRuntime().getOMPBuilder();
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
CGF.AllocaInsertPt->getParent(), CGF.AllocaInsertPt->getIterator());
- OMPBuilder.applyWorkshareLoop(
+ auto Result = OMPBuilder.applyWorkshareLoop(
CGF.Builder.getCurrentDebugLocation(), CLI, AllocaIP, NeedsBarrier,
SchedKind, ChunkSize, /*HasSimdModifier=*/false,
/*HasMonotonicModifier=*/false, /*HasNonmonotonicModifier=*/false,
/*HasOrderedClause=*/false);
+ assert(Result && "unexpected error creating workshare loop");
return;
}
@@ -4257,6 +4265,7 @@ void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
const CapturedStmt *ICS = S.getInnermostCapturedStmt();
@@ -4269,6 +4278,7 @@ void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, SubStmt, AllocaIP, CodeGenIP, "section");
+ return llvm::Error::success();
};
SectionCBVector.push_back(SectionCB);
}
@@ -4277,6 +4287,7 @@ void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, CapturedStmt, AllocaIP, CodeGenIP, "section");
+ return llvm::Error::success();
};
SectionCBVector.push_back(SectionCB);
}
@@ -4298,9 +4309,11 @@ void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
AllocaInsertPt->getParent(), AllocaInsertPt->getIterator());
- Builder.restoreIP(OMPBuilder.createSections(
+ auto Result = OMPBuilder.createSections(
Builder, AllocaIP, SectionCBVector, PrivCB, FiniCB, S.hasCancel(),
- S.getSingleClause<OMPNowaitClause>()));
+ S.getSingleClause<OMPNowaitClause>());
+ assert(Result && "unexpected error creating sections");
+ Builder.restoreIP(*Result);
return;
}
{
@@ -4326,17 +4339,21 @@ void CodeGenFunction::EmitOMPSectionDirective(const OMPSectionDirective &S) {
const Stmt *SectionRegionBodyStmt = S.getAssociatedStmt();
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
auto BodyGenCB = [SectionRegionBodyStmt, this](InsertPointTy AllocaIP,
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, SectionRegionBodyStmt, AllocaIP, CodeGenIP, "section");
+ return llvm::Error::success();
};
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- Builder.restoreIP(OMPBuilder.createSection(Builder, BodyGenCB, FiniCB));
+ auto Result = OMPBuilder.createSection(Builder, BodyGenCB, FiniCB);
+ assert(Result && "unexpected error creating section");
+ Builder.restoreIP(*Result);
return;
}
@@ -4407,17 +4424,21 @@ void CodeGenFunction::EmitOMPMasterDirective(const OMPMasterDirective &S) {
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
auto BodyGenCB = [MasterRegionBodyStmt, this](InsertPointTy AllocaIP,
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, MasterRegionBodyStmt, AllocaIP, CodeGenIP, "master");
+ return llvm::Error::success();
};
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- Builder.restoreIP(OMPBuilder.createMaster(Builder, BodyGenCB, FiniCB));
+ auto Result = OMPBuilder.createMaster(Builder, BodyGenCB, FiniCB);
+ assert(Result && "unexpected error creating master");
+ Builder.restoreIP(*Result);
return;
}
@@ -4453,18 +4474,22 @@ void CodeGenFunction::EmitOMPMaskedDirective(const OMPMaskedDirective &S) {
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
auto BodyGenCB = [MaskedRegionBodyStmt, this](InsertPointTy AllocaIP,
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, MaskedRegionBodyStmt, AllocaIP, CodeGenIP, "masked");
+ return llvm::Error::success();
};
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- Builder.restoreIP(
- OMPBuilder.createMasked(Builder, BodyGenCB, FiniCB, FilterVal));
+ auto Result =
+ OMPBuilder.createMasked(Builder, BodyGenCB, FiniCB, FilterVal);
+ assert(Result && "unexpected error creating masked");
+ Builder.restoreIP(*Result);
return;
}
@@ -4493,19 +4518,23 @@ void CodeGenFunction::EmitOMPCriticalDirective(const OMPCriticalDirective &S) {
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
auto BodyGenCB = [CriticalRegionBodyStmt, this](InsertPointTy AllocaIP,
InsertPointTy CodeGenIP) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, CriticalRegionBodyStmt, AllocaIP, CodeGenIP, "critical");
+ return llvm::Error::success();
};
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- Builder.restoreIP(OMPBuilder.createCritical(
- Builder, BodyGenCB, FiniCB, S.getDirectiveName().getAsString(),
- HintInst));
+ auto Result =
+ OMPBuilder.createCritical(Builder, BodyGenCB, FiniCB,
+ S.getDirectiveName().getAsString(), HintInst);
+ assert(Result && "unexpected error creating critical");
+ Builder.restoreIP(*Result);
return;
}
@@ -5464,11 +5493,14 @@ void CodeGenFunction::EmitOMPTaskgroupDirective(
InsertPointTy CodeGenIP) {
Builder.restoreIP(CodeGenIP);
EmitStmt(S.getInnermostCapturedStmt()->getCapturedStmt());
+ return llvm::Error::success();
};
CodeGenFunction::CGCapturedStmtInfo CapStmtInfo;
if (!CapturedStmtInfo)
CapturedStmtInfo = &CapStmtInfo;
- Builder.restoreIP(OMPBuilder.createTaskgroup(Builder, AllocaIP, BodyGenCB));
+ auto Result = OMPBuilder.createTaskgroup(Builder, AllocaIP, BodyGenCB);
+ assert(Result && "unexpected error creating taskgroup");
+ Builder.restoreIP(*Result);
return;
}
auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
@@ -6041,6 +6073,7 @@ void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &S) {
auto FiniCB = [this](InsertPointTy IP) {
OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+ return llvm::Error::success();
};
auto BodyGenCB = [&S, C, this](InsertPointTy AllocaIP,
@@ -6064,11 +6097,14 @@ void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &S) {
OMPBuilderCBHelpers::EmitOMPInlinedRegionBody(
*this, CS->getCapturedStmt(), AllocaIP, CodeGenIP, "ordered");
}
+ return llvm::Error::success();
};
OMPLexicalScope Scope(*this, S, OMPD_unknown);
- Builder.restoreIP(
- OMPBuilder.createOrderedThreadsSimd(Builder, BodyGenCB, FiniCB, !C));
+ auto Result =
+ OMPBuilder.createOrderedThreadsSimd(Builder, BodyGenCB, FiniCB, !C);
+ assert(Result && "unexpected error creating ordered");
+ Builder.restoreIP(*Result);
}
return;
}
@@ -7344,8 +7380,10 @@ void CodeGenFunction::EmitOMPCancelDirective(const OMPCancelDirective &S) {
if (IfCond)
IfCondition = EmitScalarExpr(IfCond,
/*IgnoreResultAssign=*/true);
- return Builder.restoreIP(
- OMPBuilder.createCancel(Builder, IfCondition, S.getCancelRegion()));
+ auto Result =
+ OMPBuilder.createCancel(Builder, IfCondition, S.getCancelRegion());
+ assert(Result && "unexpected error creating cancel");
+ return Builder.restoreIP(*Result);
}
}
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
index 8834c3b1f50115..d846a885bccb4b 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -538,7 +538,7 @@ class OpenMPIRBuilder {
/// A finalize callback knows about all objects that need finalization, e.g.
/// destruction, when the scope of the currently generated construct is left
/// at the time, and location, the callback is invoked.
- using FinalizeCallbackTy = std::function<void(InsertPointTy CodeGenIP)>;
+ using FinalizeCallbackTy = std::function<Error(InsertPointTy CodeGenIP)>;
struct FinalizationInfo {
/// The finalization callback provided by the last in-flight invocation of
@@ -589,15 +589,19 @@ class OpenMPIRBuilder {
/// not be split.
/// \param CodeGenIP is the insertion point at which the body code should be
/// placed.
+ ///
+ /// \return an error, if any were triggered during execution.
using BodyGenCallbackTy =
- function_ref<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP)>;
+ function_ref<Error(InsertPointTy AllocaIP, InsertPointTy CodeGenIP)>;
// This is created primarily for sections construct as llvm::function_ref
// (BodyGenCallbackTy) is not storable (as described in the comments of
// function_ref class - function_ref contains non-ownable reference
// to the callable.
+ ///
+ /// \return an error, if any were triggered during execution.
using StorableBodyGenCallbackTy =
- std::function<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP)>;
+ std::function<Error(InsertPointTy AllocaIP, InsertPointTy CodeGenIP)>;
/// Callback type for loop body code generation.
///
@@ -607,8 +611,10 @@ class OpenMPIRBuilder {
/// terminated with an unconditional branch to the loop
/// latch.
/// \param IndVar is the induction variable usable at the insertion point.
+ ///
+ /// \return an error, if any were triggered during execution.
using LoopBodyGenCallbackTy =
- function_ref<void(InsertPointTy CodeGenIP, Value *IndVar)>;
+ function_ref<Error(InsertPointTy CodeGenIP, Value *IndVar)>;
/// Callback type for variable privatization (think copy & default
/// constructor).
@@ -626,9 +632,9 @@ class OpenMPIRBuilder {
/// \param ReplVal The replacement value, thus a copy or new created version
/// of \p Inner.
///
- /// \returns The new insertion point where code generation continues and
- /// \p ReplVal the replacement value.
- using PrivatizeCallbackTy = function_ref<InsertPointTy(
+ /// \returns The new insertion point where code generation continues or an
+ /// error, and \p ReplVal the replacement value.
+ using PrivatizeCallbackTy = function_ref<Expected<InsertPointTy>(
InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &Original,
Value &Inner, Value *&ReplVal)>;
@@ -658,9 +664,10 @@ class OpenMPIRBuilder {
/// \param ThreadID Optional parameter to pass in any existing ThreadID value.
///
/// \returns The insertion point after the barrier.
- InsertPointTy createBarrier(const LocationDescription &Loc,
- omp::Directive Kind, bool ForceSimpleCall = false,
- bool CheckCancelFlag = true);
+ Expected<InsertPointTy> createBarrier(const LocationDescription &Loc,
+ omp::Directive Kind,
+ bool ForceSimpleCall = false,
+ bool CheckCancelFlag = true);
/// Generator for '#omp cancel'
///
@@ -669,8 +676,9 @@ class OpenMPIRBuilder {
/// \param CanceledDirective The kind of directive that is cancled.
///
/// \returns The insertion point after the barrier.
- InsertPointTy createCancel(const LocationDescription &Loc, Value *IfCondition,
- omp::Directive CanceledDirective);
+ Expected<InsertPointTy> createCancel(const LocationDescription &Loc,
+ Value *IfCondition,
+ omp::Directive CanceledDirective);
/// Generator for '#omp parallel'
///
@@ -685,7 +693,7 @@ class OpenMPIRBuilder {
/// \param IsCancellable Flag to indicate a cancellable parallel region.
///
/// \returns The insertion position *after* the parallel.
- IRBuilder<>::InsertPoint
+ Expected<InsertPointTy>
createParallel(const LocationDescription &Loc, InsertPointTy AllocaIP,
BodyGenCallbackTy BodyGenCB, PrivatizeCallbackTy PrivCB,
FinalizeCallbackTy FiniCB, Value *IfCondition,
@@ -711,10 +719,10 @@ class OpenMPIRBuilder {
///
/// \returns An object representing the created control flow structure which
/// can be used for loop-associated directives.
- CanonicalLoopInfo *createCanonicalLoop(const LocationDescription &Loc,
- LoopBodyGenCallbackTy BodyGenCB,
- Value *TripCount,
- const Twine &Name = "loop");
+ Expected<CanonicalLoopInfo *>
+ createCanonicalLoop(const LocationDescription &Loc,
+ LoopBodyGenCallbackTy BodyGenCB, Value *TripCount,
+ const Twine &Name = "loop");
/// Generator for the control flow structure of an OpenMP canonical loop.
///
@@ -764,12 +772,10 @@ class OpenMPIRBuilder {
///
/// \returns An object representing the created control flow structure which
/// ...
[truncated]
|
skatrak
changed the title
skatrak
force-pushed
the
omp-mlir-llvm-error-handling
branch
from
24ab62d
to
573f816
Compare
There was a problem hiding this comment.
Thank you for this PR, @skatrak, a very useful change. I have minor nits but other than that, LGTM.
skatrak
force-pushed
the
omp-mlir-llvm-error-handling
branch
from
573f816
to
8ce2262
Compare
This patch implements an approach to communicate errors between the OMPIRBuilder and its users. It introduces `llvm::Error` and `llvm::Expected` objects to replace the values returned by callbacks passed to `OMPIRBuilder` codegen functions. These functions then check the result for errors when callbacks are called and forward them back to the caller, which has the flexibility to recover, exit cleanly or dump a stack trace. This prevents a failed callback to leave the IR in an invalid state and still continue the codegen process, triggering unrelated assertions or segmentation faults. In the case of MLIR to LLVM IR translation of the 'omp' dialect, this change results in the compiler emitting errors and exiting early instead of triggering a crash for not-yet-implemented errors. The behavior in Clang and openmp-opt stays unchanged, since callbacks will continue always returning 'success'.
skatrak
force-pushed
the
omp-mlir-llvm-error-handling
branch
from
8ce2262
to
568964e
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/3878 Here is the relevant piece of the build log for the reference |
This patch implements an approach to communicate errors between the OMPIRBuilder and its users. It introduces `llvm::Error` and `llvm::Expected` objects to replace the values returned by callbacks passed to `OMPIRBuilder` codegen functions. These functions then check the result for errors when callbacks are called and forward them back to the caller, which has the flexibility to recover, exit cleanly or dump a stack trace. This prevents a failed callback to leave the IR in an invalid state and still continue the codegen process, triggering unrelated assertions or segmentation faults. In the case of MLIR to LLVM IR translation of the 'omp' dialect, this change results in the compiler emitting errors and exiting early instead of triggering a crash for not-yet-implemented errors. The behavior in Clang and openmp-opt stays unchanged, since callbacks will continue always returning 'success'.
| llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP = | ||
| OMPBuilder.createBarrier(CGF.Builder, Kind, ForceSimpleCall, | ||
| EmitChecks); | ||
| assert(AfterIP && "unexpected error creating barrier"); |
There was a problem hiding this comment.
[serious] @skatrak assert is active by LLVM_ENABLED_ASSERTS, but llvm::Error/llvm::Expected uses LLVM_ABI_BREAKING_CHECKS. In the configuration -DLLVM_ENABLED_ASSERTS=OFF -DLLVM_ABI_BREAKING_CHECKS=FORCE_ON the error check is not done, but then crashes unconditional to about unchecked errors.
There was a problem hiding this comment.
I guess then these asserts should be replaced with if (!AfterIP) llvm_unreachable(...), or is there a better way to deal with this?
There was a problem hiding this comment.
Here's the PR to address this issue: #115863. There was indeed a cleaner solution to this.
This patch implements an approach to communicate errors between the OMPIRBuilder and its users. It introduces
llvm::Errorandllvm::Expectedobjects to replace the values returned by callbacks passed toOMPIRBuildercodegen functions. These functions then check the result for errors when callbacks are called and forward them back to the caller, which has the flexibility to recover, exit cleanly or dump a stack trace.This prevents a failed callback to leave the IR in an invalid state and still continue the codegen process, triggering unrelated assertions or segmentation faults. In the case of MLIR to LLVM IR translation of the 'omp' dialect, this change results in the compiler emitting errors and exiting early instead of triggering a crash for not-yet-implemented errors. The behavior in Clang and openmp-opt stays unchanged, since callbacks will continue always returning 'success'.