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

[Layton Kifer via Phabricator via cfe-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Layton Kifer via Phabricator via cfe-commits]

laytonio added inline comments.



Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:838
+if (isValidTRECandidate(CI))
+  HasValidCandidates = true;
+  }

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Layton Kifer via Phabricator via cfe-commits]

laytonio added inline comments.



Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:816
+  for (auto  : F) {
+for (Instruction  : BB) {
+  if (AllocaInst *AI = dyn_cast()) {

You can use `for (Instruction  : instructions(F))` here.



Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:830
+  !CI->isTailCall())
+return false;
+}

Is this correct? I think we want to check these per TRE candidate in 
findTRECandidate, not just disable TRE in general if one is found.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Layton Kifer via Phabricator via cfe-commits]

laytonio added inline comments.



Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:823
+
+bool TailRecursionEliminator::canTRE(Function ) {
+  // The local stack holds all alloca instructions and all byval arguments.

There is no need to pass the function here since its a member variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82085: [TRE] markTails marks call sites as tailcalls though some of them are not.

[Layton Kifer via Phabricator via cfe-commits]

laytonio added a comment.

Relevant llvm-dev thread. Noncapture use of locals disabling 
TailRecursionElimination 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits