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

[Alexey Lapshin via Phabricator via cfe-commits]

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


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:130
+            IsNocapture = true;
+          else if (Function *CalledFunction = CB.getCalledFunction()) {
+            if (CalledFunction->getBasicBlockList().size() > 0 &&
----------------
efriedma wrote:
> Please don't add code to examine the callee; if we're not deducing nocapture appropriately, we should fix that elsewhere.
Ok. 


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:335
+        II->getIntrinsicID() == Intrinsic::assume)
+      return true;
+
----------------
efriedma wrote:
> What is the new handling for lifetime.end/assume doing?
They are just skipped. In following test case:


```
  call void @_Z5test5i(i32 %sub)
  call void @llvm.lifetime.end.p0i8(i64 24, i8* nonnull %1) #5
  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0) #5
  br label %return

```

they are generated in between call and ret. It is safe to ignore them while checking whether transformation is possible.


================
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:
> 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;

```



================
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:
> 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);
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085