[OpenMP][OMPIRBuilder] Handle non-failing calls properly #115863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
jdoerfert,
agozillon,
DominikAdamski,
TIFitis,
kparzysz and
bhandarkar-pranav
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-clang-codegen Author: Sergio Afonso (skatrak) ChangesThe preprocessor definition used to enable asserts and the one that The Patch is 76.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115863.diff 6 Files Affected:
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index d714af035d21a2..9c22103a1a75c6 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2332,11 +2332,10 @@ void CGOpenMPRuntime::emitBarrierCall(CodeGenFunction &CGF, SourceLocation Loc,
auto *OMPRegionInfo =
dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo);
if (CGF.CGM.getLangOpts().OpenMPIRBuilder) {
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createBarrier(CGF.Builder, Kind, ForceSimpleCall,
- EmitChecks);
- assert(AfterIP && "unexpected error creating barrier");
- CGF.Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createBarrier(CGF.Builder, Kind, ForceSimpleCall,
+ EmitChecks));
+ CGF.Builder.restoreIP(AfterIP);
return;
}
@@ -5932,10 +5931,9 @@ void CGOpenMPRuntime::emitTargetOutlinedFunctionHelper(
return CGF.GenerateOpenMPCapturedStmtFunction(CS, D.getBeginLoc());
};
- llvm::Error Err = OMPBuilder.emitTargetRegionFunction(
+ cantFail(OMPBuilder.emitTargetRegionFunction(
EntryInfo, GenerateOutlinedFunction, IsOffloadEntry, OutlinedFn,
- OutlinedFnID);
- assert(!Err && "unexpected error creating target region");
+ OutlinedFnID));
if (!OutlinedFn)
return;
@@ -9676,12 +9674,11 @@ static void emitTargetCallKernelLaunch(
NumTargetItems, RTArgs, NumIterations, NumTeams, NumThreads,
DynCGGroupMem, HasNoWait);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPRuntime->getOMPBuilder().emitKernelLaunch(
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPRuntime->getOMPBuilder().emitKernelLaunch(
CGF.Builder, OutlinedFnID, EmitTargetCallFallbackCB, Args, DeviceID,
- RTLoc, AllocaIP);
- assert(AfterIP && "unexpected error creating kernel launch");
- CGF.Builder.restoreIP(*AfterIP);
+ RTLoc, AllocaIP));
+ CGF.Builder.restoreIP(AfterIP);
};
if (RequiresOuterTask)
@@ -10358,12 +10355,11 @@ void CGOpenMPRuntime::emitTargetDataCalls(
InsertPointTy CodeGenIP(CGF.Builder.GetInsertBlock(),
CGF.Builder.GetInsertPoint());
llvm::OpenMPIRBuilder::LocationDescription OmpLoc(CodeGenIP);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createTargetData(
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createTargetData(
OmpLoc, AllocaIP, CodeGenIP, DeviceID, IfCondVal, Info, GenMapInfoCB,
- /*MapperFunc=*/nullptr, BodyCB, DeviceAddrCB, CustomMapperCB, RTLoc);
- assert(AfterIP && "unexpected error creating target data");
- CGF.Builder.restoreIP(*AfterIP);
+ /*MapperFunc=*/nullptr, BodyCB, DeviceAddrCB, CustomMapperCB, RTLoc));
+ CGF.Builder.restoreIP(AfterIP);
}
void CGOpenMPRuntime::emitTargetDataStandAloneCall(
diff --git a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
index 598b946ad88dbb..a794175060e41c 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
@@ -1753,14 +1753,13 @@ void CGOpenMPRuntimeGPU::emitReduction(
Idx++;
}
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createReductionsGPU(
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createReductionsGPU(
OmpLoc, AllocaIP, CodeGenIP, ReductionInfos, false, TeamsReduction,
DistributeReduction, llvm::OpenMPIRBuilder::ReductionGenCBKind::Clang,
CGF.getTarget().getGridValue(),
- C.getLangOpts().OpenMPCUDAReductionBufNum, RTLoc);
- assert(AfterIP && "unexpected error creating GPU reductions");
- CGF.Builder.restoreIP(*AfterIP);
+ C.getLangOpts().OpenMPCUDAReductionBufNum, RTLoc));
+ CGF.Builder.restoreIP(AfterIP);
return;
}
diff --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp b/clang/lib/CodeGen/CGStmtOpenMP.cpp
index 390516fea38498..ad1453ddbaaec2 100644
--- a/clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1839,11 +1839,10 @@ void CodeGenFunction::EmitOMPParallelDirective(const OMPParallelDirective &S) {
CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
AllocaInsertPt->getParent(), AllocaInsertPt->getIterator());
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP = cantFail(
OMPBuilder.createParallel(Builder, AllocaIP, BodyGenCB, PrivCB, FiniCB,
- IfCond, NumThreads, ProcBind, S.hasCancel());
- assert(AfterIP && "unexpected error creating parallel");
- Builder.restoreIP(*AfterIP);
+ IfCond, NumThreads, ProcBind, S.hasCancel()));
+ Builder.restoreIP(AfterIP);
return;
}
@@ -2135,10 +2134,8 @@ void CodeGenFunction::EmitOMPCanonicalLoop(const OMPCanonicalLoop *S) {
return llvm::Error::success();
};
- llvm::Expected<llvm::CanonicalLoopInfo *> Result =
- OMPBuilder.createCanonicalLoop(Builder, BodyGen, DistVal);
- assert(Result && "unexpected error creating canonical loop");
- llvm::CanonicalLoopInfo *CL = *Result;
+ llvm::CanonicalLoopInfo *CL =
+ cantFail(OMPBuilder.createCanonicalLoop(Builder, BodyGen, DistVal));
// Finish up the loop.
Builder.restoreIP(CL->getAfterIP());
@@ -4024,13 +4021,11 @@ static void emitOMPForDirective(const OMPLoopDirective &S, CodeGenFunction &CGF,
CGM.getOpenMPRuntime().getOMPBuilder();
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
CGF.AllocaInsertPt->getParent(), CGF.AllocaInsertPt->getIterator());
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.applyWorkshareLoop(
- CGF.Builder.getCurrentDebugLocation(), CLI, AllocaIP,
- NeedsBarrier, SchedKind, ChunkSize, /*HasSimdModifier=*/false,
- /*HasMonotonicModifier=*/false, /*HasNonmonotonicModifier=*/false,
- /*HasOrderedClause=*/false);
- assert(AfterIP && "unexpected error creating workshare loop");
+ cantFail(OMPBuilder.applyWorkshareLoop(
+ CGF.Builder.getCurrentDebugLocation(), CLI, AllocaIP, NeedsBarrier,
+ SchedKind, ChunkSize, /*HasSimdModifier=*/false,
+ /*HasMonotonicModifier=*/false, /*HasNonmonotonicModifier=*/false,
+ /*HasOrderedClause=*/false));
return;
}
@@ -4311,12 +4306,11 @@ void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
AllocaInsertPt->getParent(), AllocaInsertPt->getIterator());
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createSections(Builder, AllocaIP, SectionCBVector, PrivCB,
- FiniCB, S.hasCancel(),
- S.getSingleClause<OMPNowaitClause>());
- assert(AfterIP && "unexpected error creating sections");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createSections(
+ Builder, AllocaIP, SectionCBVector, PrivCB, FiniCB, S.hasCancel(),
+ S.getSingleClause<OMPNowaitClause>()));
+ Builder.restoreIP(AfterIP);
return;
}
{
@@ -4354,10 +4348,9 @@ void CodeGenFunction::EmitOMPSectionDirective(const OMPSectionDirective &S) {
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createSection(Builder, BodyGenCB, FiniCB);
- assert(AfterIP && "unexpected error creating section");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createSection(Builder, BodyGenCB, FiniCB));
+ Builder.restoreIP(AfterIP);
return;
}
@@ -4440,10 +4433,9 @@ void CodeGenFunction::EmitOMPMasterDirective(const OMPMasterDirective &S) {
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createMaster(Builder, BodyGenCB, FiniCB);
- assert(AfterIP && "unexpected error creating master");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createMaster(Builder, BodyGenCB, FiniCB));
+ Builder.restoreIP(AfterIP);
return;
}
@@ -4491,10 +4483,9 @@ void CodeGenFunction::EmitOMPMaskedDirective(const OMPMaskedDirective &S) {
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createMasked(Builder, BodyGenCB, FiniCB, FilterVal);
- assert(AfterIP && "unexpected error creating masked");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP = cantFail(
+ OMPBuilder.createMasked(Builder, BodyGenCB, FiniCB, FilterVal));
+ Builder.restoreIP(AfterIP);
return;
}
@@ -4535,11 +4526,11 @@ void CodeGenFunction::EmitOMPCriticalDirective(const OMPCriticalDirective &S) {
LexicalScope Scope(*this, S.getSourceRange());
EmitStopPoint(&S);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createCritical(Builder, BodyGenCB, FiniCB,
- S.getDirectiveName().getAsString(), HintInst);
- assert(AfterIP && "unexpected error creating critical");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createCritical(Builder, BodyGenCB, FiniCB,
+ S.getDirectiveName().getAsString(),
+ HintInst));
+ Builder.restoreIP(AfterIP);
return;
}
@@ -5503,10 +5494,9 @@ void CodeGenFunction::EmitOMPTaskgroupDirective(
CodeGenFunction::CGCapturedStmtInfo CapStmtInfo;
if (!CapturedStmtInfo)
CapturedStmtInfo = &CapStmtInfo;
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createTaskgroup(Builder, AllocaIP, BodyGenCB);
- assert(AfterIP && "unexpected error creating taskgroup");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPBuilder.createTaskgroup(Builder, AllocaIP, BodyGenCB));
+ Builder.restoreIP(AfterIP);
return;
}
auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
@@ -6109,10 +6099,9 @@ void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &S) {
};
OMPLexicalScope Scope(*this, S, OMPD_unknown);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createOrderedThreadsSimd(Builder, BodyGenCB, FiniCB, !C);
- assert(AfterIP && "unexpected error creating ordered");
- Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP = cantFail(
+ OMPBuilder.createOrderedThreadsSimd(Builder, BodyGenCB, FiniCB, !C));
+ Builder.restoreIP(AfterIP);
}
return;
}
@@ -7388,10 +7377,9 @@ void CodeGenFunction::EmitOMPCancelDirective(const OMPCancelDirective &S) {
if (IfCond)
IfCondition = EmitScalarExpr(IfCond,
/*IgnoreResultAssign=*/true);
- llvm::OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPBuilder.createCancel(Builder, IfCondition, S.getCancelRegion());
- assert(AfterIP && "unexpected error creating cancel");
- return Builder.restoreIP(*AfterIP);
+ llvm::OpenMPIRBuilder::InsertPointTy AfterIP = cantFail(
+ OMPBuilder.createCancel(Builder, IfCondition, S.getCancelRegion()));
+ return Builder.restoreIP(AfterIP);
}
}
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index d2e4dc1c85dfd2..5d9a7784d9e68c 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -4225,7 +4225,11 @@ OpenMPIRBuilder::applyStaticChunkedWorkshareLoop(DebugLoc DL,
// Create outer "dispatch" loop for enumerating the chunks.
BasicBlock *DispatchEnter = splitBB(Builder, true);
Value *DispatchCounter;
- Expected<CanonicalLoopInfo *> LoopResult = createCanonicalLoop(
+
+ // It is safe to assume this didn't return an error because the callback
+ // passed into createCanonicalLoop is the only possible error source, and it
+ // always returns success.
+ CanonicalLoopInfo *DispatchCLI = cantFail(createCanonicalLoop(
{Builder.saveIP(), DL},
[&](InsertPointTy BodyIP, Value *Counter) {
DispatchCounter = Counter;
@@ -4233,15 +4237,7 @@ OpenMPIRBuilder::applyStaticChunkedWorkshareLoop(DebugLoc DL,
},
FirstChunkStart, CastedTripCount, NextChunkStride,
/*IsSigned=*/false, /*InclusiveStop=*/false, /*ComputeIP=*/{},
- "dispatch");
- if (!LoopResult) {
- // It is safe to assume this didn't return an error because the callback
- // passed into createCanonicalLoop is the only possible error source, and it
- // always returns success. Need to still cast the result into bool to avoid
- // runtime errors.
- llvm_unreachable("unexpected error creating canonical loop");
- }
- CanonicalLoopInfo *DispatchCLI = *LoopResult;
+ "dispatch"));
// Remember the BasicBlocks of the dispatch loop we need, then invalidate to
// not have to preserve the canonical invariant.
@@ -6542,16 +6538,12 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::createTargetData(
};
bool RequiresOuterTargetTask = Info.HasNoWait;
- if (!RequiresOuterTargetTask) {
- Error Err = TaskBodyCB(/*DeviceID=*/nullptr, /*RTLoc=*/nullptr,
- /*TargetTaskAllocaIP=*/{});
- assert(!Err && "TaskBodyCB expected to succeed");
- } else {
- InsertPointOrErrorTy AfterIP =
- emitTargetTask(TaskBodyCB, DeviceID, SrcLocInfo, AllocaIP,
- /*Dependencies=*/{}, Info.HasNoWait);
- assert(AfterIP && "TaskBodyCB expected to succeed");
- }
+ if (!RequiresOuterTargetTask)
+ cantFail(TaskBodyCB(/*DeviceID=*/nullptr, /*RTLoc=*/nullptr,
+ /*TargetTaskAllocaIP=*/{}));
+ else
+ cantFail(emitTargetTask(TaskBodyCB, DeviceID, SrcLocInfo, AllocaIP,
+ /*Dependencies=*/{}, Info.HasNoWait));
} else {
Function *BeginMapperFunc = getOrCreateRuntimeFunctionPtr(
omp::OMPRTL___tgt_target_data_begin_mapper);
diff --git a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
index 9e25620710fc84..153f5c7510efae 100644
--- a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
+++ b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
@@ -1178,15 +1178,12 @@ struct OpenMPOpt {
OpenMPIRBuilder::LocationDescription Loc(
InsertPointTy(ParentBB, ParentBB->end()), DL);
- OpenMPIRBuilder::InsertPointOrErrorTy SeqAfterIP =
- OMPInfoCache.OMPBuilder.createMaster(Loc, BodyGenCB, FiniCB);
- assert(SeqAfterIP && "Unexpected error creating master");
+ OpenMPIRBuilder::InsertPointTy SeqAfterIP = cantFail(
+ OMPInfoCache.OMPBuilder.createMaster(Loc, BodyGenCB, FiniCB));
+ cantFail(
+ OMPInfoCache.OMPBuilder.createBarrier(SeqAfterIP, OMPD_parallel));
- OpenMPIRBuilder::InsertPointOrErrorTy BarrierAfterIP =
- OMPInfoCache.OMPBuilder.createBarrier(*SeqAfterIP, OMPD_parallel);
- assert(BarrierAfterIP && "Unexpected error creating barrier");
-
- BranchInst::Create(SeqAfterBB, SeqAfterIP->getBlock());
+ BranchInst::Create(SeqAfterBB, SeqAfterIP.getBlock());
LLVM_DEBUG(dbgs() << TAG << "After sequential inlining " << *OuterFn
<< "\n");
@@ -1256,12 +1253,11 @@ struct OpenMPOpt {
OriginalFn->getEntryBlock().getFirstInsertionPt());
// Create the merged parallel region with default proc binding, to
// avoid overriding binding settings, and without explicit cancellation.
- OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPInfoCache.OMPBuilder.createParallel(
+ OpenMPIRBuilder::InsertPointTy AfterIP =
+ cantFail(OMPInfoCache.OMPBuilder.createParallel(
Loc, AllocaIP, BodyGenCB, PrivCB, FiniCB, nullptr, nullptr,
- OMP_PROC_BIND_default, /* IsCancellable */ false);
- assert(AfterIP && "Unexpected error creating parallel");
- BranchInst::Create(AfterBB, AfterIP->getBlock());
+ OMP_PROC_BIND_default, /* IsCancellable */ false));
+ BranchInst::Create(AfterBB, AfterIP.getBlock());
// Perform the actual outlining.
OMPInfoCache.OMPBuilder.finalize(OriginalFn);
@@ -1297,12 +1293,10 @@ struct OpenMPOpt {
if (CI != MergableCIs.back()) {
// TODO: Remove barrier if the merged parallel region includes the
// 'nowait' clause.
- OpenMPIRBuilder::InsertPointOrErrorTy AfterIP =
- OMPInfoCache.OMPBuilder.createBarrier(
- InsertPointTy(NewCI->getParent(),
- NewCI->getNextNode()->getIterator()),
- OMPD_parallel);
- assert(AfterIP && "Unexpected error creating barrier");
+ cantFail(OMPInfoCache.OMPBuilder.createBarrier(
+ InsertPointTy(NewCI->getParent(),
+ NewCI->getNextNode()->getIterator()),
+ OMPD_parallel));
}
CI->eraseFromParent();
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index 630cd03c688012..dfdb5d69501ed4 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -235,10 +235,9 @@ class OpenMPIRBuilderTest : public testing::Test {
return Error::success();
};
- Expected<CanonicalLoopInfo *> LoopResult =
- OMPBuilder.createCanonicalLoop(Loc, LoopBodyGenCB, CastedTripCount);
- assert(LoopResult && "unexpected error");
- CanonicalLoopInfo *Loop = *LoopResult;
+
+ CanonicalLoopInfo *Loop = cantFail(
+ OMPBuilder.createCanonicalLoop(Loc, LoopBodyGenCB, CastedTripCount));
// Finalize the function.
Builder.restoreIP(Loop->getAfterIP());
@@ -345,18 +344,14 @@ TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
IRBuilder<> Builder(BB);
- OpenMPIRBuilder::InsertPointOrErrorTy BarrierIP1 =
- OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
- assert(BarrierIP1 && "unexpected error");
+ cantFail(OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for));
EXPECT_TRUE(M->global_empty());
EXPECT_EQ(M->size(), 1U);
EXPECT_EQ(F->size(), 1U);
EXPECT_EQ(BB->size(), 0U);
OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
- OpenMPIRBuilder::InsertPointOrErrorTy BarrierIP2 =
- OMPBuilder.createBarrier(Loc, OMPD_for);
- assert(BarrierIP2 && "unexpected error");
+ cantFail(OMPBuilder.createBarrier(Loc, OMPD_for));
EXPECT_FALSE(M->global_empty());
EXPECT_EQ(M->size(), 3U);
EXPECT_EQ(F->size(), 1U);
@@ -399,10 +394,9 @@ TEST_F(OpenMPIRBuilderTest, CreateCancel) {
IRBuilder<> Builder(BB);
OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
- OpenMPIRBuilder::InsertPointOrErrorTy NewIP =
- OMPBuilder.createCancel(Loc, nullptr, OMPD_parallel);
- assert(NewIP && "unexpected error");
- Builder.restoreIP(*NewIP);
+ OpenMPIRBuilder::InsertPointTy NewIP =
+ cantFail(OMPBuilder.createCancel(Loc, nullptr, OMPD_parallel));
+ Builder.restoreIP(NewIP);
EXPECT_FALSE(M->global_empty());
EXPECT_EQ(M->size(), 4U);
EXPECT_EQ(F->size(), 4U);
@@ -424,7 +418,7 @@ TEST_F(OpenMPIRBuilderTest, CreateCancel) {
EXPECT_EQ(Cancel->getNumUses(), 1U);
Instruction *CancelBBTI = Cancel->getParent()->getTerminator();
EXPECT_EQ(CancelBBTI->getNumSuccessors(), 2U);
- EXPECT_EQ(CancelBBTI->getSuccessor(0), NewIP->getBlock());
+ EXPECT_EQ(CancelBBTI->getSuccessor(...
[truncated]
|
|
Ideas:
Consider mentioning which configuration |
skatrak
force-pushed
the
ompirbuilder-err-assert
branch
from
7584acb to
a66140f
Compare
|
Thank you @Meinersbur for your comments. I just updated the PR description and replaced |
Meinersbur
reviewed
Nov 14, 2024
| assert(LoopResult && "unexpected error"); | ||
| CanonicalLoopInfo *Loop = *LoopResult; | ||
|
|
||
| CanonicalLoopInfo *Loop = *expectedToOptional( |
There was a problem hiding this comment.
Derefencting a std::nullopt is undefined behaviour, so potentially the same as cantFail.
The comment of expectedToOptional indicated that it is discurraged to use it.
Comment on lines
347
to
348
| ASSERT_TRUE(expectedToOptional( | ||
| OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for))); |
There was a problem hiding this comment.
llvm::Expected as well as std::optional are convertable to bool, but because ASSERT_TRUE actually converts to AssertionResult, only the latter works. So this is the shorter version of the same (which I have been using so far):
Suggested change
| ASSERT_TRUE(expectedToOptional( | |
| OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for))); | |
| ASSERT_TRUE((bool)OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for)); |
I hoped you could find something nicer. Some tests define their own macro ASSERT_NO_ERROR. LLVM itself defines ASSERT_THAT_EXPECTED to be used in GTest. None of these return the value in-case of no-error, so ugly to use, but seem to be the canonical way to do the check. How about a helper function similar to cantFail, but guaranteed to crash instead of undefined behaviour?
template<typename T>
T successOrAbort(Expected<T> &&ValOrErr) {
if (ValOrErr)
return std::move(*ValOrErr);
report_fatal_error(ValOrErr.takeError());
}
Something really cool would properly integreate into the GTest envorinment.
template<typename T>
T successOrAbort(Expected<T> &&ValOrErr) {
if (ValOrErr)
return std::move(*ValOrErr);
GTEST_NONFATAL_FAILURE_("Expected<T> contains error value"); // Might be considered internal to GTest.
abort(); // Cannot continue without a value to unwrap
}
or make it a macro such we can customize the message and use GTEST_MESSAGE_AT_ and report the affected line number.
There was a problem hiding this comment.
I made another revision to avoid translating to std::optional in order to discard errors, and instead handle them through some new local macros. I'm not against adding these functions that you propose, but that doesn't seem like a good thing to do as part of this PR. Perhaps something that can be added separately and then potentially update this and other tests to use it.
Meinersbur
approved these changes
Jan 7, 2025
The preprocessor definition used to enable asserts and the one that `llvm::Error` and `llvm::Expected` use to ensure all created instances are checked are not the same. By making these checks inside of an `assert` in cases where errors are not expected, certain build configurations would trigger runtime failures (e.g. `-DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_UNREACHABLE_OPTIMIZE=ON`). The `llvm::cantFail()` function, which was intended for this use case, is used by this patch in place of `assert` to prevent these runtime failures. In tests, new preprocessor definitions based on `ASSERT_THAT_EXPECTED` and `EXPECT_THAT_EXPECTED` are used instead, to avoid silent failures in release builds.
skatrak
force-pushed
the
ompirbuilder-err-assert
branch
from
a338fa6 to
23af823
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/8435 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The preprocessor definition used to enable asserts and the one that
llvm::Errorandllvm::Expecteduse to ensure all created instances are checked are not the same. By making these checks inside of anassertin cases where errors are not expected, certain build configurations would trigger runtime failures (e.g.-DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_UNREACHABLE_OPTIMIZE=ON).The
llvm::cantFail()function, which was intended for this use case, is used by this patch in place ofassertto prevent these runtime failures. For tests, thellvm::expectedToOptional()in combination withASSERT_TRUEandstd::optional's builtin asserts are used instead, to avoid silent failures in release builds.