Skip to content

[FastISel] Support unreachable with NoTrapAfterNoReturn #118296

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

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Dec 2, 2024

Currently FastISel triggers a fallback if there is an unreachable terminator and the TrapUnreachable option is enabled (the ISD::TRAP selection does not actually work).

Add handling for NoTrapAfterNoReturn, in which case we don't actually need to emit a trap. The test is just there to make sure there is no FastISel fallback (which is why I'm not testing the case without noreturn). We have other tests that check the actual unreachable codegen variations.

Currently FastISel triggers a fallback if there is an unreachable
terminator and the TrapUnreachable option is enabled (the ISD::TRAP
selection does not actually work).

Add handling for NoTrapAfterNoReturn, in which case we don't
actually need to emit a trap. The test is just there to make sure
there is no FastISel fallback (which is why I'm not testing the
case without noreturn). We have other tests that check the actual
unreachable codegen variations.
@nikic nikic requested review from RKSimon, aengelke and topperc December 2, 2024 13:56
@llvmbot llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Dec 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 2, 2024

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Nikita Popov (nikic)

Changes

Currently FastISel triggers a fallback if there is an unreachable terminator and the TrapUnreachable option is enabled (the ISD::TRAP selection does not actually work).

Add handling for NoTrapAfterNoReturn, in which case we don't actually need to emit a trap. The test is just there to make sure there is no FastISel fallback (which is why I'm not testing the case without noreturn). We have other tests that check the actual unreachable codegen variations.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/FastISel.cpp (+12-4)
  • (added) llvm/test/CodeGen/X86/no-trap-after-noreturn-fastisel.ll (+13)
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index eede879e7e80d1..d5551758c073e9 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -1851,11 +1851,19 @@ bool FastISel::selectOperator(const User *I, unsigned Opcode) {
     return false;
   }
 
-  case Instruction::Unreachable:
-    if (TM.Options.TrapUnreachable)
+  case Instruction::Unreachable: {
+    if (TM.Options.TrapUnreachable) {
+      if (TM.Options.NoTrapAfterNoreturn) {
+        const auto *Call =
+            dyn_cast_or_null<CallInst>(cast<Instruction>(I)->getPrevNode());
+        if (Call && Call->doesNotReturn())
+          return true;
+      }
+
       return fastEmit_(MVT::Other, MVT::Other, ISD::TRAP) != 0;
-    else
-      return true;
+    }
+    return true;
+  }
 
   case Instruction::Alloca:
     // FunctionLowering has the static-sized case covered.
diff --git a/llvm/test/CodeGen/X86/no-trap-after-noreturn-fastisel.ll b/llvm/test/CodeGen/X86/no-trap-after-noreturn-fastisel.ll
new file mode 100644
index 00000000000000..5149209f79d156
--- /dev/null
+++ b/llvm/test/CodeGen/X86/no-trap-after-noreturn-fastisel.ll
@@ -0,0 +1,13 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -O0 -trap-unreachable -no-trap-after-noreturn -fast-isel-abort=3 < %s | FileCheck %s
+
+declare void @foo()
+
+define void @noreturn_unreachable() nounwind {
+; CHECK-LABEL: noreturn_unreachable:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    callq foo@PLT
+  call void @foo() noreturn
+  unreachable
+}

if (TM.Options.TrapUnreachable) {
if (TM.Options.NoTrapAfterNoreturn) {
const auto *Call =
dyn_cast_or_null<CallInst>(cast<Instruction>(I)->getPrevNode());
Copy link
Contributor

Choose a reason for hiding this comment

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

CallBase?

; CHECK-NEXT: callq foo@PLT
call void @foo() noreturn
unreachable
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check invoke case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

invoke is a terminator, so it can't be followed by unreachable. (Same is true for callbr.)

Copy link
Contributor

@aengelke aengelke left a comment

Choose a reason for hiding this comment

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

LGTM. Was there any concrete use case where this caused a noticeable amount of fallbacks?

@nikic
Copy link
Contributor Author

nikic commented Dec 3, 2024

LGTM. Was there any concrete use case where this caused a noticeable amount of fallbacks?

Yes, this is one of the more common cause of fallbacks for Rust code. For the test case I'm looking at the distribution looks like this:

array(7) {
  ["invoke"]=>
  int(7870)
  ["unreachable"]=>
  int(3160)
  ["landingpad"]=>
  int(630)
  ["args"]=>
  int(566)
  ["ret"]=>
  int(397)
  ["other"]=>
  int(324)
  ["switch"]=>
  int(237)
}

@nikic nikic merged commit b2df007 into llvm:main Dec 3, 2024
11 checks passed
@nikic nikic deleted the fastisel-notrapafternoreturn branch December 3, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants