laytonio added inline comments.

Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:838
+        if (isValidTRECandidate(CI))
+          HasValidCandidates = true;
+      }
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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to