Inliner: Don't clear tail flags when creating allocas for byval args

[rnk]

Summary

This is essentially a partial revert of r105255, which fixed PR7272.
The test case it introduced[1] still passes:

test/Transforms/Inline/2010-05-31-ByvalTailcall.ll

I think this was misdiagnosed. The call to @ext in the test case should
never have been marked 'tail' by instcombine, tailcallelim, or any other
optimization because it captures a byval argument, which is using memory
from the caller. That's against the LangRef rules for tail calls. It
looks like the optimization that added the tail flag has been fixed in
the meantime, because the test still passes.

[chandlerc]

The change looks fine, but can you add a positive test case?

[rnk]

• Add tests
• Actually fix the latent bug in tailcallelim, it wasn't fixed yet

[chandlerc]

Hah! Nice though. Nits only, LGTM to commit with these fixes.

test/Transforms/Inline/byval-tail-call.ll

4–5 ↗

(On Diff #8656)

Nice. If you're doing this much surgery here, I would rename it without the date so that people can find the test case more easily (byval_tail_call.ll or whatever).

test/Transforms/TailCallElim/basic.ll

149 ↗

(On Diff #8656)

missing :

[rnk]

test/Transforms/Inline/byval-tail-call.ll

4–5 ↗

(On Diff #8656)

I did. :)

test/Transforms/TailCallElim/basic.ll

149 ↗

(On Diff #8656)

woops, thanks!

[rnk]

Closed by commit rL206789 (authored by @rnk).

llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp

for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I) {
         PointerMayBeCaptured(AI, &ACT);
         // If any allocas are captured, exit.
         if (ACT.Captured)
           return false;
       }
     }
   }
 
+  // If any byval or inalloca args are captured, exit. They are also allocated
+  // in our stack frame.
+  for (Argument &Arg : F.args()) {
+    if (Arg.hasByValOrInAllocaAttr())
+      PointerMayBeCaptured(&Arg, &ACT);
+    if (ACT.Captured)
+      return false;
+  }
+
   // Second pass, change any tail recursive calls to loops.
   //
   // FIXME: The code generator produces really bad code when an 'escaping
   // alloca' is changed from being a static alloca to being a dynamic alloca.
   // Until this is resolved, disable this transformation if that would ever
   // happen.  This bug is PR962.
   if (ACT.UsesAlloca.empty()) {
     for (Function::iterator BB = F.begin(), E = F.end(); BB != E; ++BB) {

llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp

for (Function::const_arg_iterator I = CalledFunc->arg_begin(),
 
       // When byval arguments actually inlined, we need to make the copy implied
       // by them explicit.  However, we don't do this if the callee is readonly
       // or readnone, because the copy would be unneeded: the callee doesn't
       // modify the struct.
       if (CS.isByValArgument(ArgNo)) {
         ActualArg = HandleByValArgument(ActualArg, TheCall, CalledFunc, IFI,
                                         CalledFunc->getParamAlignment(ArgNo+1));
- 
-        // Calls that we inline may use the new alloca, so we need to clear
-        // their 'tail' flags if HandleByValArgument introduced a new alloca and
-        // the callee has calls.
-        if (ActualArg != *AI) {
-          MustClearTailCallFlags = true;
+        if (ActualArg != *AI)
           ByValInit.push_back(std::make_pair(ActualArg, (Value*) *AI));
-        }
 
-      }
-
       VMap[I] = ActualArg;
     }
 
     // We want the inliner to prune the code as it copies.  We would LOVE to
     // have no dead or constant instructions leftover after inlining occurs
     // (which can happen, e.g., because an argument was constant), but we'll be
     // happy with whatever the cloner can do.
     CloneAndPruneFunctionInto(Caller, CalledFunc, VMap, 

llvm/trunk/test/Transforms/Inline/2010-05-31-ByvalTailcall.ll

-; RUN: opt < %s -tailcallelim -inline -instcombine -dse -S | FileCheck %s
-; PR7272
-
-; When inlining through a byval call site, the inliner creates allocas which may
-; be used by inlined calls, so any inlined calls need to have their 'tail' flags
-; cleared.  If not then you can get nastiness like with this testcase, where the
-; (inlined) call to 'ext' in 'foo' was being passed an uninitialized value.
-
-target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32"
-target triple = "i386-pc-linux-gnu"
-
-declare void @ext(i32*)
-
-define void @bar(i32* byval %x) {
-  call void @ext(i32* %x)
-  ret void
-}
-
-define void @foo(i32* %x) {
-; CHECK-LABEL: define void @foo(
-; CHECK: llvm.lifetime.start
-; CHECK: store i32 %2, i32* %x
-  call void @bar(i32* byval %x)
-  ret void
-}

llvm/trunk/test/Transforms/Inline/byval-tail-call.ll

+; RUN: opt < %s -tailcallelim -inline -instcombine -dse -S | FileCheck %s
+; PR7272
+
+; Calls that capture byval parameters cannot be marked as tail calls. Other
+; tails that don't capture byval parameters can still be tail calls.
+
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32"
+target triple = "i386-pc-linux-gnu"
+
+declare void @ext(i32*)
+
+define void @bar(i32* byval %x) {
+  call void @ext(i32* %x)
+  ret void
+}
+
+define void @foo(i32* %x) {
+; CHECK-LABEL: define void @foo(
+; CHECK: llvm.lifetime.start
+; CHECK: store i32 %2, i32* %x
+  call void @bar(i32* byval %x)
+  ret void
+}
+
+define internal void @qux(i32* byval %x) {
+  call void @ext(i32* %x)
+  tail call void @ext(i32* null)
+  ret void
+}
+define void @frob(i32* %x) {
+; CHECK-LABEL: define void @frob(
+; CHECK: alloca i32
+; CHECK: {{^ *}}call void @ext(
+; CHECK: tail call void @ext(i32* null)
+; CHECK: ret void
+  tail call void @qux(i32* byval %x)
+  ret void
+}

llvm/trunk/test/Transforms/TailCallElim/basic.ll

cond_true:
   call void @use(i32* %B2)
   call i32* @test8(i32* %B, i1 false)
   ret i32* null
 cond_false:
   call void @use2_nocapture(i32* %A, i32* %B)
   call void @noarg()
   ret i32* null
 }
+
+; Don't tail call if a byval arg is captured.
+define void @test9(i32* byval %a) {
+; CHECK-LABEL: define void @test9(
+; CHECK: {{^ *}}call void @use(
+  call void @use(i32* %a)
+  ret void
+}