[llvm] [CodeGen] Avoid generating trap instructions after exception restore intrinsics (PR #109560)

[via llvm-commits]

================
@@ -3125,6 +3125,14 @@ bool IRTranslator::translateUnreachable(const User &U, MachineIRBuilder &MIRBuil
     // Do not emit an additional trap instruction.
     if (Call->isNonContinuableTrap())
       return true;
+    // Do not emit trap instructions after EH_RETURN intrinsics.
+    // This can cause problems later down the line when other machine passes
+    // attempt to use the last instruction in a BB to determine terminator behavior.
+    if (const auto *II = dyn_cast<IntrinsicInst>(Call)) {
+      const auto IID = II->getIntrinsicID();
+      if (IID == Intrinsic::eh_return_i32 || IID == Intrinsic::eh_return_i64)
+        return true;
+    }
----------------
duk-37 wrote:

> Should these really be terminators in the first place? Can the insert point just be adjusted?

Yes and no, respectively. They're old exception-handling intrinsics that lower down to `ISD::EH_RETURN` through the SelectionDAG, which is then lowered to target-specific `EH_RETURN` opcodes across different architectures. These opcodes are all marked as "return instructions" in various `.td` files.

The problem becomes that these are not actually return instructions, they're intrinsics. When they're lowered through, we visit an `unreachable` instruction after the `EH_RETURN` opcode is emitted through the intrinsic, meaning that if `trap-unreachable` is enabled we emit a `TRAP` instruction after a terminator. You can see this behavior here: https://godbolt.org/z/5br41sWqf

We also get blocks that where `MBB.isReturnBlock()` returns false where it would have returned true before, as this function checks the last instruction in a block to figure out whether it's a "return block" or not. This is used, for example, in [PrologEpilogInsertion](https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/PrologEpilogInserter.cpp#L428) to determine where to restore registers.

I'm not quite sure if this is a miscompile - maybe the restores aren't necessary for correct behavior - but returning false in `isReturnBlock` is definitely not what we want to happen here. Also, the restore behavior is also explicitly checked in tests, so I'm definitely not going to bank on that.

Looking at this again, I'm not sure they're even supported in GlobalISel on any architecture, but it would be good to keep this behavior consistent between instruction selection backends.

https://github.com/llvm/llvm-project/pull/109560