Skip to content
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.

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

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

Closed

Conversation

goldsteinn
Copy link
Contributor

Since some of the users of CodeExtractor like HotColdSplitting run
late in the pipeline, returns are not cleaned to unreachable. So,
just emit unreachable directly if the function is noreturn.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 10, 2024

@llvm/pr-subscribers-llvm-transforms

Author: None (goldsteinn)

Changes

Since some of the users of CodeExtractor like HotColdSplitting run
late in the pipeline, returns are not cleaned to unreachable. So,
just emit unreachable directly if the function is noreturn.


Full diff: https://github.com/llvm/llvm-project/pull/84682.diff

11 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/CodeExtractor.cpp (+17-11)
  • (modified) llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll (+2-2)
  • (modified) llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll (+3-3)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected (+2-2)
  • (modified) llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected (+2-2)
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index ab2d25c3f17c7d..ee4e52cefe7fe1 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -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);
+        }))
+      newFunction->setDoesNotReturn();
+  }
+
   newFunction->insert(newFunction->end(), newRootNode);
 
   // Create scalar and aggregate iterators to name all of the arguments we
@@ -1390,12 +1402,16 @@ CallInst *CodeExtractor::emitCallAndSwitchStatement(Function *newFunction,
   Type *OldFnRetTy = TheSwitch->getParent()->getParent()->getReturnType();
   switch (NumExitBlocks) {
   case 0:
+      // If fn is no return, end with an unreachable terminator.
+    if (newFunction->doesNotReturn()) {
+      (void)new UnreachableInst(Context, TheSwitch->getIterator());
+    }
     // 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()) {
+    else if (OldFnRetTy->isVoidTy()) {
       ReturnInst::Create(Context, nullptr, TheSwitch->getIterator());  // Return void
     } else if (OldFnRetTy == TheSwitch->getCondition()->getType()) {
       // return what we have
@@ -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())) {
     newFunction->dump();
     report_fatal_error("verification of newFunction failed!");
diff --git a/llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll b/llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll
index 0c055981260b2b..55013aa96551d0 100644
--- a/llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll
+++ b/llvm/test/Transforms/HotColdSplit/outline-disjoint-diamonds.ll
@@ -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
diff --git a/llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll b/llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll
index 6b12207a32dff8..d2c41376f6b2c8 100644
--- a/llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll
+++ b/llvm/test/Transforms/IROutliner/outlining-no-return-functions.ll
@@ -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(
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected
index 02d8870c61365c..775649cee0e764 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
index 05e57774c260e8..a8086ae2157096 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected
index 36bcbe32e03634..57de350dec15e8 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
index db7c692a7def98..696d5c6edee6f3 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected
index 10399951e7fbcd..5275870b4088d7 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
index 0001790e66ab37..712ccb25ae8577 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected
index e05a57d06413b9..b5b12b770522bf 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected
@@ -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
 ;
@@ -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
 ;
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
index 17be222f15c125..7e2b991cd6fccb 100644
--- a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
@@ -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
 ;
@@ -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
 ;

Copy link

github-actions bot commented Mar 10, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@goldsteinn goldsteinn force-pushed the goldsteinn/code-extractor-unreachable branch from 1ef8926 to 01acea2 Compare March 10, 2024 23:44
@goldsteinn
Copy link
Contributor Author

ping.

// 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.

});
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.

… with `unreachable`

Since some of the users of `CodeExtractor` like `HotColdSplitting` run
late in the pipeline, returns are not cleaned to `unreachable`. So,
just emit `unreachable` directly if the function is `noreturn`.
@goldsteinn goldsteinn force-pushed the goldsteinn/code-extractor-unreachable branch from 01acea2 to ad4a027 Compare March 15, 2024 17:36
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
… with `unreachable`

Since some of the users of `CodeExtractor` like `HotColdSplitting` run
late in the pipeline, returns are not cleaned to `unreachable`. So,
just emit `unreachable` directly if the function is `noreturn`.

Closes llvm#84682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants