[PATCH] D82085: [TRE] allow TRE for non-capturing calls.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:825
+  // The local stack holds all alloca instructions and all byval arguments.
+  AllocaDerivedValueTracker Tracker;
+  for (Argument &Arg : F.args()) {
----------------
avl wrote:
> efriedma wrote:
> > Do you have to redo the AllocaDerivedValueTracker analysis?  Is it not enough that the call you're trying to TRE is marked "tail"?
> >Do you have to redo the AllocaDerivedValueTracker analysis?
> 
> AllocaDerivedValueTracker analysis(done in markTails) could be reused here. 
> But marking, done in markTails(), looks like separate tasks. i.e. it is better 
> to make TRE not depending on markTails(). There is a review for this - https://reviews.llvm.org/D60031
> Thus such separation looks useful(To not reuse result of markTails but have it computed inplace).
> 
> > Is it not enough that the call you're trying to TRE is marked "tail"?
> 
> It is not enough that call which is subject to TRE is marked "Tail".
> It also should be checked that other calls does not capture pointer to local stack: 
> 
> ```
> // do not do TRE if any pointer to local stack has escaped.
> if (!Tracker.EscapePoints.empty())
>    return false;
> 
> ```
> 
> It is not enough that call which is subject to TRE is marked "Tail". It also should be checked that other calls does not capture pointer to local stack:

If there's an escaped pointer to the local stack, we wouldn't infer "tail" in the first place, would we?


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:852
+
+        // Do not do TRE if exists recursive calls which are not last calls.
+        if (!isTailBlock(CI->getParent()) ||
----------------
avl wrote:
> efriedma wrote:
> > I thought we had some tests where we TRE in the presence of recursive calls, like a simple recursive fibonacci.  Am I misunderstanding this?
> right, there is a testcase for fibonacchi:
> 
> llvm/test/Transforms/TailCallElim/accum_recursion.ll:@test3_fib
> 
> areAllLastFuncCallsRecursive() checking works well for fibonacci testcase:
> 
> 
> ```
> return fib(x-1)+fib(x-2);
> 
> ```
> 
> Since, Last funcs call chain is : fib()->fib()->ret. 
> That check should prevent from such cases:
> 
> 
> ```
> return fib(x-1)+another_call()+fib(x-2);
> ```
> 
> 
> That check should prevent from such cases: return fib(x-1)+another_call()+fib(x-2);

Why do we need to prevent this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82085/new/

https://reviews.llvm.org/D82085