Avoid emitting unreachable SP adjustments after throw

In 172eee9, we tried to avoid these by modelling the callee as
internally resetting the stack pointer.

However, for the majority of functions with reserved stack frames, this
would lead LLVM to emit extra SP adjustments to undo the callee's
internal adjustment. This lead us to fix the problem further on down the
pipeline in eliminateCallFramePseudoInstr. In 5b79e60, I added
use a heuristic to try to detect when the adjustment would be
unreachable.

This heuristic is imperfect, and when exception handling is involved, it
fails to fire. The new test is an example of this. Simply throwing an
exception with an active cleanup emits dead SP adjustments after the
throw. Not only are they dead, but if they were executed, they would be
incorrect, so they are confusing.

This change essentially reverts 172eee9 and makes the 5b79e60
heuristic responsible for preventing unreachable stack adjustments. This
means we may emit unreachable stack adjustments for functions using EH
with unreserved call frames, but that is not very many these days. Back
in 2016 when this change was added, we were focused on 32-bit, which we
observed to have fewer reserved frames.

Fixes PR45064

Reviewed By: hans

Differential Revision: https://reviews.llvm.org/D75712

Author: rnk

llvm/lib/Target/X86/X86FrameLowering.cpp

@@ -3009,6 +3009,12 @@ eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
   I = MBB.erase(I);
   auto InsertPos = skipDebugInstructionsForward(I, MBB.end());
 
+  // Try to avoid emitting dead SP adjustments if the block end is unreachable,
+  // typically because the function is marked noreturn (abort, throw,
+  // assert_fail, etc).
+  if (isDestroy && blockEndIsUnreachable(MBB, I))
+    return I;
+
   if (!reserveCallFrame) {
     // If the stack pointer can be changed after prologue, turn the
     // adjcallstackup instruction into a 'sub ESP, <amt>' and the
@@ -3091,13 +3097,7 @@ eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
     return I;
   }
 
-  if (isDestroy && InternalAmt && !blockEndIsUnreachable(MBB, I)) {
-    // If we are performing frame pointer elimination and if the callee pops
-    // something off the stack pointer, add it back.  We do this until we have
-    // more advanced stack pointer tracking ability.
-    // We are not tracking the stack pointer adjustment by the callee, so make
-    // sure we restore the stack pointer immediately after the call, there may
-    // be spill code inserted between the CALL and ADJCALLSTACKUP instructions.
+  if (InternalAmt) {
     MachineBasicBlock::iterator CI = I;
     MachineBasicBlock::iterator B = MBB.begin();
     while (CI != B && !std::prev(CI)->isCall())

llvm/lib/Target/X86/X86ISelLowering.cpp

@@ -4362,12 +4362,6 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   else
     NumBytesForCalleeToPop = 0;  // Callee pops nothing.
 
-  if (CLI.DoesNotReturn && !getTargetMachine().Options.TrapUnreachable) {
-    // No need to reset the stack after the call if the call doesn't return. To
-    // make the MI verify, we'll pretend the callee does it for us.
-    NumBytesForCalleeToPop = NumBytes;
-  }
-
   // Returns a flag for retval copy to use.
   if (!IsSibcall) {
     Chain = DAG.getCALLSEQ_END(Chain,

llvm/test/CodeGen/X86/noreturn-call-win64.ll

@@ -1,5 +1,8 @@
 ; RUN: llc < %s -mtriple=x86_64-windows-msvc | FileCheck %s
 
+%struct.MakeCleanup = type { i8 }
+%eh.ThrowInfo = type { i32, i32, i32, i32 }
+
 ; Function Attrs: noinline nounwind optnone uwtable
 define dso_local i32 @foo() {
 entry:
@@ -51,3 +54,58 @@ declare dso_local i32 @cond()
 declare dso_local void @abort1() noreturn
 declare dso_local void @abort2() noreturn
 declare dso_local void @abort3() noreturn
+
+define dso_local void @throw_exception() uwtable personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  %o = alloca %struct.MakeCleanup, align 1
+  %call = invoke i32 @cond()
+          to label %invoke.cont unwind label %ehcleanup
+
+invoke.cont:                                      ; preds = %entry
+  %cmp1 = icmp eq i32 0, %call
+  br i1 %cmp1, label %if.then, label %if.end
+
+if.then:                                          ; preds = %invoke.cont
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null)
+          to label %unreachable unwind label %ehcleanup
+
+if.end:                                           ; preds = %invoke.cont
+  %call2 = invoke i32 @cond()
+          to label %invoke.cont1 unwind label %ehcleanup
+
+invoke.cont1:                                     ; preds = %if.end
+  %cmp2 = icmp eq i32 0, %call2
+  br i1 %cmp2, label %if.then3, label %if.end4
+
+if.then3:                                         ; preds = %invoke.cont1
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null)
+          to label %unreachable unwind label %ehcleanup
+
+if.end4:                                          ; preds = %invoke.cont1
+  call void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup* nonnull %o)
+  ret void
+
+ehcleanup:                                        ; preds = %if.then3, %if.end, %if.then, %entry
+  %cp = cleanuppad within none []
+  call void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup* nonnull %o) [ "funclet"(token %cp) ]
+  cleanupret from %cp unwind to caller
+
+unreachable:                                      ; preds = %if.then3, %if.then
+  unreachable
+}
+
+declare dso_local i32 @__CxxFrameHandler3(...)
+declare dso_local void @_CxxThrowException(i8*, %eh.ThrowInfo*)
+declare dso_local void @"??1MakeCleanup@@QEAA@XZ"(%struct.MakeCleanup*)
+
+; CHECK-LABEL: throw_exception:
+; CHECK: callq cond
+; CHECK: je
+; CHECK: callq cond
+; CHECK: je
+; CHECK: retq
+; CHECK: callq _CxxThrowException
+; CHECK-NOT: {{(addq|subq) .*, %rsp}}
+; CHECK: callq _CxxThrowException
+; CHECK-NOT: {{(addq|subq) .*, %rsp}}
+; CHECK: .seh_handlerdata