[llvm] r222354 - Fix tail recursion elimination

[Arnaud A. de Grandmaison]

Author: aadg
Date: Wed Nov 19 07:32:51 2014
New Revision: 222354

URL: http://llvm.org/viewvc/llvm-project?rev=222354&view=rev
Log:
Fix tail recursion elimination

When the BasicBlock containing the return instrution has a PHI with 2
incoming values, FoldReturnIntoUncondBranch will remove the no longer
used incoming value and remove the no longer needed phi as well. This
leaves us with a BB that no longer has a PHI, but the subsequent call
to FoldReturnIntoUncondBranch from FoldReturnAndProcessPred will not
remove the return instruction (which still uses the result of the call
instruction). This prevents EliminateRecursiveTailCall to remove
the value, as it is still being used in a basicblock which has no
predecessors.

The basicblock can not be erased on the spot, because its iterator is
still being used in runTRE.

This issue was exposed when removing the threshold on size for lifetime
marker insertion for named temporaries in clang. The testcase is a much
reduced version of peelOffOuterExpr(const Expr*, const ExplodedNode *)
from clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp.

Added:
    llvm/trunk/test/Transforms/TailCallElim/EraseBB.ll
Modified:
    llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp

Modified: llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp?rev=222354&r1=222353&r2=222354&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp Wed Nov 19 07:32:51 2014
@@ -404,18 +404,28 @@ bool TailCallElim::runTRE(Function &F) {
   // 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.
+  SmallVector<BasicBlock*, 8> BBToErase;
   for (Function::iterator BB = F.begin(), E = F.end(); BB != E; ++BB) {
     if (ReturnInst *Ret = dyn_cast<ReturnInst>(BB->getTerminator())) {
       bool Change = ProcessReturningBlock(Ret, OldEntry, TailCallsAreMarkedTail,
                                           ArgumentPHIs, !CanTRETailMarkedCall);
-      if (!Change && BB->getFirstNonPHIOrDbg() == Ret)
+      if (!Change && BB->getFirstNonPHIOrDbg() == Ret) {
         Change = FoldReturnAndProcessPred(BB, Ret, OldEntry,
                                           TailCallsAreMarkedTail, ArgumentPHIs,
                                           !CanTRETailMarkedCall);
+        // FoldReturnAndProcessPred may have emptied some BB. Remember to
+        // erase them.
+        if (Change && BB->empty())
+          BBToErase.push_back(BB);
+
+      }
       MadeChange |= Change;
     }
   }
 
+  for (auto BB: BBToErase)
+    BB->eraseFromParent();
+
   // If we eliminated any tail recursions, it's possible that we inserted some
   // silly PHI nodes which just merge an initial value (the incoming operand)
   // with themselves.  Check to see if we did and clean up our mess if so.  This
@@ -823,8 +833,20 @@ bool TailCallElim::FoldReturnAndProcessP
     if (CallInst *CI = FindTRECandidate(BI, CannotTailCallElimCallsMarkedTail)){
       DEBUG(dbgs() << "FOLDING: " << *BB
             << "INTO UNCOND BRANCH PRED: " << *Pred);
-      EliminateRecursiveTailCall(CI, FoldReturnIntoUncondBranch(Ret, BB, Pred),
-                                 OldEntry, TailCallsAreMarkedTail, ArgumentPHIs,
+      ReturnInst *RI = FoldReturnIntoUncondBranch(Ret, BB, Pred);
+
+      // Cleanup: if all predecessors of BB have been eliminated by
+      // FoldReturnIntoUncondBranch, we would like to delete it, but we
+      // can not just nuke it as it is being used as an iterator by our caller.
+      // Just empty it, and the caller will erase it when it is safe to do so.
+      // It is important to empty it, because the ret instruction in there is
+      // still using a value which EliminateRecursiveTailCall will attempt
+      // to remove.
+      if (!BB->hasAddressTaken() && pred_begin(BB) == pred_end(BB))
+        BB->getInstList().clear();
+
+      EliminateRecursiveTailCall(CI, RI, OldEntry, TailCallsAreMarkedTail,
+                                 ArgumentPHIs,
                                  CannotTailCallElimCallsMarkedTail);
       ++NumRetDuped;
       Change = true;

Added: llvm/trunk/test/Transforms/TailCallElim/EraseBB.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/TailCallElim/EraseBB.ll?rev=222354&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/TailCallElim/EraseBB.ll (added)
+++ llvm/trunk/test/Transforms/TailCallElim/EraseBB.ll Wed Nov 19 07:32:51 2014
@@ -0,0 +1,26 @@
+; RUN: opt -tailcallelim -S < %s 2>&1 | FileCheck %s
+
+; CHECK: add nsw i32
+; CHECK-NEXT: br label
+; CHECK: add nsw i32
+; CHECK-NEXT: br label
+; CHECK-NOT: Uses remain when a value is destroyed
+define i32 @test(i32 %n) {
+entry:
+  %cmp = icmp slt i32 %n, 2
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %v1 = add nsw i32 %n, -2
+  %call1 = tail call i32 @test(i32 %v1)
+  br label %return
+
+if.else:                                          ; preds = %entry
+  %v2 = add nsw i32 %n, 4
+  %call2 = tail call i32 @test(i32 %v2)
+  br label %return
+
+return:                                           ; preds = %if.end, %if.else
+  %retval = phi i32 [ %call1, %if.then ], [ %call2, %if.else ]
+  ret i32 %retval
+}