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

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked an inline comment as done.
avl added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:838
+        if (isValidTRECandidate(CI))
+          HasValidCandidates = true;
+      }
----------------
laytonio wrote:
> avl wrote:
> > laytonio wrote:
> > > Is there any reason to find and validate candidates now only to have to redo it when we actually perform the eliminations? If so, is there any reason this needs to follow a different code path than findTRECandidate? findTRECandidate is doing the same checks, except for canMoveAboveCall and canTransformAccumulatorRecursion, which should probably be refactored into findTRECandidate from eliminateCall anyway. If not then all of this code goes away and we're left with the same canTRE as in trunk.
> > We are enumerating all instructions here, so we could understand if there are not TRE candidates and stop earlier. That is the reason for doing it here.
> > 
> > I agree that findTRECandidate should be refactored to have the same checks as here. 
> > 
> > What do you think is better to do:
> > 
> > - leave early check for TRE candidates in canTRE or remove it
> > - refactor findTRECandidate or leave it as is
> > 
> > ?
> Yes we are iterating all the instructions here but, unless I am missing something, we would literally just be doing the checks twice for no reason. Look at it this way, best case scenario we have to check all possible candidates once, find none and we're done. Worst case, we check all possible candidates once, find one and have to check all possible candidates a second time. Where as if we remove the early checks we only ever have to check the candidates once. So we wouldn't really be stopping any earlier.
> 
> As for refactoring findTRECandidate, I do think that should be done and we should strive to move all the failure conditions out of eliminateCall in order to avoid having to fold a return only to find out we didn't need to. But, I think that is out of the scope of this change, and if we do decide to keep the early checks here then we should say that findTRECandidate does a good enough job to consider this function as having valid candidates.
>Yes we are iterating all the instructions here but, unless I am missing something, we would literally just be doing the checks twice for no reason. Look at it this way, best case scenario we have to check all possible candidates once, find none and we're done. Worst case, we check all possible candidates once, find one and have to check all possible candidates a second time. Where as if we remove the early checks we only ever have to check the candidates once. So we wouldn't really be stopping any earlier.

yes. we would do check twice if there are TRE candidates. my idea was that number of cases when TRE is applicable less then when TRE is not applicable. Thus we would do double instruction navigation more often than double check for candidates. But, I did not measure real impact. Thus, let`s return old logic here as you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085