[PATCH] Teach the inliner how to preserve musttail invariants

[Reid Kleckner]

================
Comment at: lib/IR/Instructions.cpp:327
@@ -326,3 +326,3 @@
   setAttributes(CI.getAttributes());
-  setTailCall(CI.isTailCall());
+  setTailCallKind(CI.getTailCallKind());
   setCallingConv(CI.getCallingConv());
----------------
Chandler Carruth wrote:
> Please commit this separately with a separate test? Maybe a unittest?
Done.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:483
@@ -480,1 +482,3 @@
 
+static bool isPrecededByMustTailCall(const Instruction *Inst) {
+  const Instruction *Prev = Inst->getPrevNode();
----------------
Chandler Carruth wrote:
> This is called in a context that needs it to not go truly linear... And it doesn't. I almost want to call it 'isImmediatelyPrecededByMustTailCall', but the bitcast thing makes that a lie. Maybe just some comments?
Comments sound good to me.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:687-690
@@ +686,6 @@
+  //
+  //    f -> musttail g -> musttail h  ==>  f -> musttail g
+  //    f -> musttail g ->     tail h  ==>  f ->     tail g
+  //    f ->          g -> musttail h  ==>  f ->          g
+  //    f ->          g ->     tail h  ==>  f ->          g
+  //
----------------
Chandler Carruth wrote:
> I assume the after cases should have "h"? Actually, shouldn't these be "f -> g -> f" => "f -> f"?
Yeah. Let's go with f -> g -> f.  That's the example described in the paragraph above.  The reader can extrapolate a larger cyclical call graph.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:692-693
@@ +691,4 @@
+  //
+  // Also, calls inlined through a 'nounwind' call site should be marked
+  // 'nounwind'.
+  bool InlinedMustTailCalls = false;
----------------
Chandler Carruth wrote:
> I would sink this comment to the loop body.
done

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:744-745
@@ +743,4 @@
+      for (ReturnInst *RI : Returns) {
+        // Don't insert lifetime.end calls after a musttail call, the allocas
+        // are trivially dead.
+        if (InlinedMustTailCalls && isPrecededByMustTailCall(RI))
----------------
Chandler Carruth wrote:
> Do all of the passes using lifetime markers know this?
> 
> It might be useful to separate this into its own patch with its own test case.
Sure. I'll split this into a followup patch to highlight the test case that fails without it.  It fixes the byval and dynalloca test cases.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:795
@@ +794,3 @@
+      // Change users of the return value of the call.  For a musttail call
+      // site, the only real user should be a return instruction with an
+      // optional bitcast.  Avoid chains of bitcasts.
----------------
Chandler Carruth wrote:
> You say "should" but then "assert" below. I think the should is correct though...
The assert was correct.  I reworked this to be simpler.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:804
@@ +803,3 @@
+          RV = OldCast->getOperand(0);
+        assert(cast<CallInst>(RV)->isMustTailCall());
+
----------------
Chandler Carruth wrote:
> So, I don't think you can *always* assert. What about a musttail call to a function that just happens to return the result of a non-musttail call? Above, you set the flag if any call is musttail, so it seems you should just continue if this call site isn't a musttail.
> 
> Similarly, I don't think you can assert that there will be a callinst here.
We are looping over the second partition here, i.e. returns that are preceded by musttail.  So, all of these things have to be true, or there are bugs in isPrecededByMustTailCall.

It sounds like this was confusing, so I'll restructure it without partition.  That was silly.

http://reviews.llvm.org/D3491