Skip to content

Commit 9afb1c5

Browse files
committed
Revert "Outline non returning functions unless a longjmp"
This reverts commit 2079798. This patch (https://reviews.llvm.org/D69257) cannot complete a stage2 build due to the change: ``` CI->getCalledFunction()->getName().contains("longjmp") ``` There are several concrete issues here: - The callee may not be a function, so `getCalledFunction` can assert. - The called value may not have a name, so `getName` can assert. - There's no distinction made between "my_longjmp_test_helper" and the actual longjmp libcall. At a higher level, there's a serious layering problem here. The splitting pass makes policy decisions in a general way (e.g. based on attributes or profile data). Special-casing certain names breaks the layering. It subverts the work of library maintainers (who may now need to opt-out of unexpected optimization behavior for any affected functions) and can lead to inconsistent optimization behavior (as not all llvm passes special-case ".*longjmp.*" in the same way). The patch may need significant revision to address these issues. But the immediate issue is that this crashes while compiling llvm's unit tests in a stage2 build (due to the `getName` problem).
1 parent 6bec45e commit 9afb1c5

File tree

6 files changed

+5
-528
lines changed

6 files changed

+5
-528
lines changed

llvm/lib/Transforms/IPO/HotColdSplitting.cpp

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,7 @@ bool blockEndsInUnreachable(const BasicBlock &BB) {
113113
return !(isa<ReturnInst>(I) || isa<IndirectBrInst>(I));
114114
}
115115

116-
bool unlikelyExecuted(BasicBlock &BB, ProfileSummaryInfo *PSI,
117-
BlockFrequencyInfo *BFI) {
116+
bool unlikelyExecuted(BasicBlock &BB) {
118117
// Exception handling blocks are unlikely executed.
119118
if (BB.isEHPad() || isa<ResumeInst>(BB.getTerminator()))
120119
return true;
@@ -127,19 +126,12 @@ bool unlikelyExecuted(BasicBlock &BB, ProfileSummaryInfo *PSI,
127126
return true;
128127

129128
// The block is cold if it has an unreachable terminator, unless it's
130-
// preceded by a call to a (possibly warm) noreturn call (e.g. longjmp);
131-
// in the case of a longjmp, if the block is cold according to
132-
// profile information, we mark it as unlikely to be executed as well.
129+
// preceded by a call to a (possibly warm) noreturn call (e.g. longjmp).
133130
if (blockEndsInUnreachable(BB)) {
134131
if (auto *CI =
135132
dyn_cast_or_null<CallInst>(BB.getTerminator()->getPrevNode()))
136-
if (CI->hasFnAttr(Attribute::NoReturn)) {
137-
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI))
138-
return (II->getIntrinsicID() != Intrinsic::eh_sjlj_longjmp) ||
139-
(BFI && PSI->isColdBlock(&BB, BFI));
140-
return !CI->getCalledFunction()->getName().contains("longjmp") ||
141-
(BFI && PSI->isColdBlock(&BB, BFI));
142-
}
133+
if (CI->hasFnAttr(Attribute::NoReturn))
134+
return false;
143135
return true;
144136
}
145137

@@ -599,7 +591,7 @@ bool HotColdSplitting::outlineColdRegions(Function &F, bool HasProfileSummary) {
599591
continue;
600592

601593
bool Cold = (BFI && PSI->isColdBlock(BB, BFI)) ||
602-
(EnableStaticAnalysis && unlikelyExecuted(*BB, PSI, BFI));
594+
(EnableStaticAnalysis && unlikelyExecuted(*BB));
603595
if (!Cold)
604596
continue;
605597

llvm/test/Transforms/HotColdSplit/longjmp-nosplit.ll

Lines changed: 0 additions & 97 deletions
This file was deleted.

llvm/test/Transforms/HotColdSplit/longjmp-split.ll

Lines changed: 0 additions & 132 deletions
This file was deleted.

llvm/test/Transforms/HotColdSplit/sjlj-nosplit.ll

Lines changed: 0 additions & 103 deletions
This file was deleted.

0 commit comments

Comments
 (0)