X86: Don't reset the stack after calls that don't return (PR27117)

[hans]

Summary

Here's a stab at removing these unnecessary post-call stack adjustments.

[rnk]

include/llvm/Target/TargetLowering.h

2525

Call can be an invoke, in which case it terminates, in which case this may call isa<> on an end iterator, which is bad. I think the easy way to avoid this case is check for !Call.isInvoke() && ....

lib/Target/X86/X86ISelLowering.cpp

3384

We should kill that TargetOption eventually...

[majnemer]

include/llvm/Target/TargetLowering.h

2525

Instead of isa<UnreachableInst>(++Call.getInstruction()->getIterator()) you could do isa<UnreachableInst>(Call.getInstruction()->getNextNode()) which is a little shorter (and simpler IMO).

[rnk]

include/llvm/Target/TargetLowering.h

2525

I think you have to null check getNextNode, though.

[hans]

include/llvm/Target/TargetLowering.h

2525

Oh right, I forgot invokes are terminators. Adding the !Call.isInvoke() check.

2525

Thanks! I'll do that.

2525

But not if we know the instruction isn't last in the BB right?

[hans]

Addressing comments.

[rnk]

lgtm

[rnk]

It looks like we still have this problem, despite many attempts to fix it. rG5b79e603d3b7a29940df6580d6f62b0e9bd339c0 / D66905, for example.

If you enable exceptions, my checks aren't enough. This C++ code for example:

struct MakeCleanup {
  ~MakeCleanup();
};
bool cond();
void foo() {
  MakeCleanup o;
  if (cond())
    throw;
  if (cond())
    throw;
}

I think we should look into removing this x86isellowering code.

include/llvm/Target/TargetLowering.h

struct CallLoweringInfo {
     }
 
     CallLoweringInfo &setCallee(Type *ResultType, FunctionType *FTy,
                                 SDValue Target, ArgListTy &&ArgsList,
                                 ImmutableCallSite &Call) {
       RetTy = ResultType;
 
       IsInReg = Call.paramHasAttr(0, Attribute::InReg);
-      DoesNotReturn = Call.doesNotReturn();
+      DoesNotReturn =
+          Call.doesNotReturn() ||
+          (!Call.isInvoke() &&
+           isa<UnreachableInst>(Call.getInstruction()->getNextNode()));
       IsVarArg = FTy->isVarArg();
       IsReturnValueUsed = !Call.getInstruction()->use_empty();
       RetSExt = Call.paramHasAttr(0, Attribute::SExt);
       RetZExt = Call.paramHasAttr(0, Attribute::ZExt);
 
       Callee = Target;
 
       CallConv = Call.getCallingConv();

lib/Target/X86/X86ISelLowering.cpp

else if (!Is64Bit && !canGuaranteeTCO(CallConv) &&
     // If this is a call to a struct-return function, the callee
     // pops the hidden struct pointer, so we have to push it back.
     // This is common for Darwin/X86, Linux & Mingw32 targets.
     // For MSVC Win32 targets, the caller pops the hidden struct pointer.
     NumBytesForCalleeToPop = 4;
   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,
                                DAG.getIntPtrConstant(NumBytesToPop, dl, true),
                                DAG.getIntPtrConstant(NumBytesForCalleeToPop, dl,
                                                      true),
                                InFlag, dl);
     InFlag = Chain.getValue(1);

test/CodeGen/X86/noreturn-call.ll

+; RUN: llc < %s -mtriple=i686-pc-win32 | FileCheck %s
+
+define void @test1(i32 %c) {
+; CHECK-LABEL: test1:
+entry:
+  %0 = alloca i8, i32 %c
+  %tobool = icmp eq i32 %c, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.end:
+  call void @g(i8* %0)
+  ret void
+
+if.then:
+  call void @crash(i8* %0)
+  unreachable
+; CHECK: calll _crash
+; There is no need to adjust the stack after the call, since
+; the function is noreturn and that code will therefore never run.
+; CHECK-NOT: add
+; CHECK-NOT: pop
+}
+
+define void @test2(i32 %c) {
+; CHECK-LABEL: test2:
+entry:
+  %0 = alloca i8, i32 %c
+  %tobool = icmp eq i32 %c, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.end:
+  call void @g(i8* %0)
+  ret void
+
+if.then:
+  call void @crash2(i8* %0)
+  unreachable
+; CHECK: calll _crash2
+; Even though _crash2 is not marked noreturn, it is in practice because
+; of the "unreachable" right after it. This happens e.g. when falling off
+; a non-void function after a call.
+; CHECK-NOT: add
+; CHECK-NOT: pop
+}
+
+declare void @crash(i8*) noreturn
+declare void @crash2(i8*)
+declare void @g(i8*)