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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:94
 /// If it contains any dynamic allocas, returns false.
 static bool canTRE(Function &F) {
   // Because of PR962, we don't TRE dynamic allocas.
----------------
If we're not going to try to do TRE at all on calls not marked "tail", we can probably drop this check.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:808
   // Until this is resolved, disable this transformation if that would ever
   // happen.  This bug is PR962.
   for (Function::iterator BBI = F.begin(), E = F.end(); BBI != E; /*in loop*/) {
----------------
Can you move this FIXME into a more appropriate spot?


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:335
+        II->getIntrinsicID() == Intrinsic::assume)
+      return true;
+
----------------
avl wrote:
> 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.
It makes sense we can ignore lifetime.end on an alloca: we know the call doesn't refer to the alloca.  (Maybe we should check that the pointer argument is pointing at an alloca?  That should usually be true anyway, but better to be on the safe side, I guess.)

I don't think it's safe to hoist assume without additional checks; I think we'd need to check that the call is marked "willreturn"?

Since this is sort of tricky, I'd prefer to split this off into a followup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085