[PATCH] Teach the inliner how to preserve musttail invariants

[Reid Kleckner]

Thanks, committing.  I'll update the follow-on, http://reviews.llvm.org/D3630, in a sec.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:482-483
@@ -480,1 +481,4 @@
 
+/// Returns true if the given return instruction is immediately preceded by a
+/// musttail call or a bitcast immediately preceded by a musttail call.
+static CallInst *getPrecedingMustTailCall(ReturnInst *RI) {
----------------
Chandler Carruth wrote:
> Stale comment.
fixed

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:694-705
@@ -663,1 +693,14 @@
 
+  // We need to reduce the strength of any inlined tail calls.  For musttail, we
+  // have to avoid introducing potential unbounded stack growth.  For example,
+  // if functions 'f' and 'g' are mutually recursive with musttail, we can
+  // inline 'g' into 'f' so long as we preserve musttail on the cloned call to
+  // 'f'.  If either the inlined call site or the cloned call site is *not*
+  // musttail, the program already has one frame of stack growth, so it's safe
+  // to remove musttail.  Here is a table of example transformations:
+  //
+  //    f -> musttail g -> musttail f  ==>  f -> musttail f
+  //    f -> musttail g ->     tail f  ==>  f ->     tail f
+  //    f ->          g -> musttail f  ==>  f ->          f
+  //    f ->          g ->     tail f  ==>  f ->          f
+  bool InlinedMustTailCalls = false;
----------------
Chandler Carruth wrote:
> Would it make sense to sink this comment down to the TailCallKind bit below?
Seems reasonable.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:780-784
@@ -719,1 +779,7 @@
+    for (ReturnInst *RI : Returns) {
+      // Don't insert llvm.stackrestore calls after a musttail call, since the
+      // tail call will clear the whole stack frame.
+      if (InlinedMustTailCalls && getPrecedingMustTailCall(RI))
+        continue;
+      IRBuilder<>(RI).CreateCall(StackRestore, SavedPtr);
     }
----------------
Chandler Carruth wrote:
> It's really surprising to me that this applies to *all* dynamic allocas rather than just to inalloca arguments... Maybe I'm just missing the point, but I'd appreciate a comment clarifying why this makes sense.
> 
> Is there a test case that covers this? I can't tell if some of the CHECK-NEXT ones actually cover this case.
I'm not really following.

Here's what could happen when inlining this fragment:
  %a = alloca i8, i32 %n
  call void @use_a(i8* %a)
  musttail call void @bar()
  ret void

Without this change, after inlining, we get this:
  %sp = call i8* @llvm.stacksave()
  %a = alloca i8, i32 %n
  call void @use_a(i8* %a)
  musttail call void @bar()
  call void @llvm.stackrestore(i8*)
  ret void

First, this fails musttail verification, because now the musttail call isn't in tail position.

The stackrestore is completely redundant with the ret.  Unlike other rets, this ret is going to remain in the inlined function.  It isn't removed later and merged back into the normal control flow.

Would it make more sense to reframe the comment to talk about the fact that this ret isn't going a way and stackrestore before ret is pointless?  The way I think about it is that we just did a musttail call which already did the work of clearing the stack, so there's no reason to restore the stack.

Anyway, this should have been split into the @lifetime.end change, because it's covered by those tests.  I'll go do that.

http://reviews.llvm.org/D3491