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

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked 3 inline comments as done.
avl added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:823
+
+bool TailRecursionEliminator::canTRE(Function &F) {
+  // The local stack holds all alloca instructions and all byval arguments.
----------------
laytonio wrote:
> There is no need to pass the function here since its a member variable.
Ok.


================
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()) {
----------------
efriedma wrote:
> 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?
If function receives pointer to alloca then it would not be marked with "Tail". Then we do not have a possibility to understand whether this function receives pointer to alloca but does not capture it:

```
void test(int recurseCount)
{
    if (recurseCount == 0) return;
    int temp = 10;
    globalIncrement(&temp);
    test(recurseCount - 1);
}
```

test - marked with Tail.
globalIncrement - not marked with Tail. But TRE could be done since it does not capture pointer. But if it will capture the pointer then we could not do TRE. So we need to check !Tracker.EscapePoints.empty().





================
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()) ||
----------------
efriedma wrote:
> 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?
We do not. I misunderstood the canTransformAccumulatorRecursion(). That check could be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085