[PATCH] D52895: [TailCallElim] Enable marking of calls with byval as tails

[Nick Lewycky via Phabricator via llvm-commits]

nicholas added a comment.

I don't understand the logical model behind byval, but I'll review the change to TRE under the assumption that the goal of the patch is correct.



================
Comment at: lib/Transforms/Scalar/TailRecursionElimination.cpp:138
             CS.isDataOperand(U) && CS.doesNotCapture(CS.getDataOperandNo(U));
         callUsesLocalStack(CS, IsNocapture);
         if (IsNocapture) {
----------------
I think you may also want to change the logic in callUsesLocalStack; this tracks users of an alloca when the callee could leak/escape this alloca pointer value to a place where a future call could obtain it, even if it isn't passed in as an argument (say, written to a global here and read back in a future call). If the pointer received by the callee is not truly the pointer to the alloca, then this couldn't happen. This case really matters for cycles in the CFG, see the comment on DeferredTails.


================
Comment at: test/Transforms/Inline/byval-tail-call.ll:44
+
+; If a call passes a byval parameter as byval the call can be marked as a
+; tail.
----------------
This "if X then Y" makes it sound like we don't need to check any other properties in this case, like
  if (call passes byval) {
    markTail(call);
  }

I think you meant something more like "A byval parameter does not block a call from being marked tail." or "A call with a byval parameter may still be marked as a tail."


Repository:
  rL LLVM

https://reviews.llvm.org/D52895