Skip to content

[CodeExtractor] Terminate callsite blocks to new noreturn functions with unreachable #84682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[CodeExtractor] Terminate callsite blocks to new noreturn functions with unreachable #84682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions llvm/lib/Transforms/Utils/CodeExtractor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,18 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,

newFunction->addFnAttr(Attr);
}

if (NumExitBlocks == 0) {
// Mark the new function `noreturn` if applicable. Terminators which resume
// exception propagation are treated as returning instructions. This is to
// avoid inserting traps after calls to outlined functions which unwind.
if (none_of(Blocks, [](const BasicBlock *BB) {
const Instruction *Term = BB->getTerminator();
return isa<ReturnInst>(Term) || isa<ResumeInst>(Term);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be safer to do all_of unreachableinst. This requires maintaining a list of all possible returning terminators (though I guess this was just moved)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think that should be a follow up patch.
FWIW I tested this w/ hotcoldsplitting enabled (+ split all no-returns) on llvm-test-suite/spec/bootstrap and saw no issues.

}))
newFunction->setDoesNotReturn();
}

newFunction->insert(newFunction->end(), newRootNode);

// Create scalar and aggregate iterators to name all of the arguments we
Expand Down Expand Up @@ -1392,19 +1404,23 @@ CallInst *CodeExtractor::emitCallAndSwitchStatement(Function *newFunction,
case 0:
// There are no successors (the block containing the switch itself), which
// means that previously this was the last part of the function, and hence
// this should be rewritten as a `ret'

// Check if the function should return a value
if (OldFnRetTy->isVoidTy()) {
ReturnInst::Create(Context, nullptr, TheSwitch->getIterator()); // Return void
// this should be rewritten as a `ret` or `unreachable`.
if (newFunction->doesNotReturn()) {
// If fn is no return, end with an unreachable terminator.
(void)new UnreachableInst(Context, TheSwitch->getIterator());
} else if (OldFnRetTy->isVoidTy()) {
// We have no return value.
ReturnInst::Create(Context, nullptr,
TheSwitch->getIterator()); // Return void
} else if (OldFnRetTy == TheSwitch->getCondition()->getType()) {
// return what we have
ReturnInst::Create(Context, TheSwitch->getCondition(), TheSwitch->getIterator());
ReturnInst::Create(Context, TheSwitch->getCondition(),
TheSwitch->getIterator());
} else {
// Otherwise we must have code extracted an unwind or something, just
// return whatever we want.
ReturnInst::Create(Context,
Constant::getNullValue(OldFnRetTy), TheSwitch->getIterator());
ReturnInst::Create(Context, Constant::getNullValue(OldFnRetTy),
TheSwitch->getIterator());
}

TheSwitch->eraseFromParent();
Expand Down Expand Up @@ -1895,16 +1911,6 @@ CodeExtractor::extractCodeRegion(const CodeExtractorAnalysisCache &CEAC,

fixupDebugInfoPostExtraction(*oldFunction, *newFunction, *TheCall);

// Mark the new function `noreturn` if applicable. Terminators which resume
// exception propagation are treated as returning instructions. This is to
// avoid inserting traps after calls to outlined functions which unwind.
bool doesNotReturn = none_of(*newFunction, [](const BasicBlock &BB) {
const Instruction *Term = BB.getTerminator();
return isa<ReturnInst>(Term) || isa<ResumeInst>(Term);
});
if (doesNotReturn)
newFunction->setDoesNotReturn();

LLVM_DEBUG(if (verifyFunction(*newFunction, &errs())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see why this does this, the regular post-pass verification would catch this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested a simple case w.o this (using HotColdSplitting). If we don't add noreturn here, it never gets added in defaultO3 pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB the comment around when HotColdSplitting is enabled:

  // Split out cold code. Splitting is done late to avoid hiding context from
  // other optimizations and inadvertently regressing performance. The tradeoff
  // is that this has a higher code size cost than splitting early.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't talking about anything the pass does, I mean the verifyFunction call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, not sure what the need for that is.

newFunction->dump();
report_fatal_error("verification of newFunction failed!");
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

; CHECK-LABEL: define {{.*}}@fun
; CHECK: call {{.*}}@fun.cold.1(
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: call {{.*}}@fun.cold.2(
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
define void @fun() {
entry:
br i1 undef, label %A.then, label %A.else
Expand Down
6 changes: 3 additions & 3 deletions llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ bb1:
; CHECK-LABEL: @f1(
; CHECK-NEXT: bb:
; CHECK-NEXT: call void @outlined_ir_func_0()
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
;
;
; CHECK-LABEL: @f2(
; CHECK-NEXT: bb:
; CHECK-NEXT: call void @outlined_ir_func_0()
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
;
;
; CHECK-LABEL: @f3(
; CHECK-NEXT: bb:
; CHECK-NEXT: call void @outlined_ir_func_0()
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
;
;
; CHECK-LABEL: define internal void @outlined_ir_func_0(
Expand Down
4 changes: 2 additions & 2 deletions ...st/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ declare void @_Z10sideeffectv()
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand All @@ -52,7 +52,7 @@ declare void @_Z10sideeffectv()
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @bar.cold.1() #[[ATTR2]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions .../UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ declare void @_Z10sideeffectv()
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand All @@ -55,7 +55,7 @@ declare void @_Z10sideeffectv()
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @bar.cold.1() #[[ATTR2]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions .../tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ define void @foo(i32) {
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand All @@ -36,7 +36,7 @@ define void @bar(i32) {
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @bar.cold.1() #[[ATTR2]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions ...pdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ define void @foo(i32) {
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand All @@ -39,7 +39,7 @@ define void @bar(i32) {
; CHECK-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; CHECK: codeRepl:
; CHECK-NEXT: call void @bar.cold.1() #[[ATTR2]]
; CHECK-NEXT: ret void
; CHECK-NEXT: unreachable
; CHECK: exit:
; CHECK-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions ...teTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ declare void @_Z10sideeffectv()
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand All @@ -53,7 +53,7 @@ declare void @_Z10sideeffectv()
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @bar.cold.1() #[[ATTR2]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions ...ecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ declare void @_Z10sideeffectv()
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand All @@ -56,7 +56,7 @@ declare void @_Z10sideeffectv()
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @bar.cold.1() #[[ATTR2]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions ...TestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ define void @foo(i32) {
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand All @@ -37,7 +37,7 @@ define void @bar(i32) {
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @bar.cold.1() #[[ATTR2]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand Down
4 changes: 2 additions & 2 deletions ...ks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ define void @foo(i32) {
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @foo.cold.1() #[[ATTR2:[0-9]+]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand All @@ -40,7 +40,7 @@ define void @bar(i32) {
; REUSE-NEXT: br i1 [[TMP2]], label [[CODEREPL:%.*]], label [[EXIT:%.*]]
; REUSE: codeRepl:
; REUSE-NEXT: call void @bar.cold.1() #[[ATTR2]]
; REUSE-NEXT: ret void
; REUSE-NEXT: unreachable
; REUSE: exit:
; REUSE-NEXT: ret void
;
Expand Down