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

[Robert Lougher via Phabricator via llvm-commits]

rob.lougher added a comment.

Hi Nicholas and Reid,

Thanks for the reviews. I'll make the suggested changes tomorrow (later on today as it's now past midnight in the UK).

Thanks,
Rob.



================
Comment at: lib/Transforms/Scalar/TailRecursionElimination.cpp:138
             CS.isDataOperand(U) && CS.doesNotCapture(CS.getDataOperandNo(U));
         callUsesLocalStack(CS, IsNocapture);
         if (IsNocapture) {
----------------
nicholas wrote:
> 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.
If I understand correctly, you're worried about the byval "pointer" escaping, as calls after the escape point cannot be marked tail? This isn't an issue, as the alloca is copied into some target specific area, and isn't passed directly (this is why it is simply skipped - an alloca with byval is a copy, so it isn't leaking or using the alloca). If a callee did try to save it for a future call it would be equally broken irrespective of whether the future call was marked tail or not. 


================
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.
----------------
nicholas wrote:
> 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."
Yes, on hindsight it does sound confusing.  First there's your point, and second the important point is that the byval is both in and out.  How about:

"A byval parameter passed into a function which is passed out as byval does not block the call from being marked as tail". 


================
Comment at: test/Transforms/Inline/byval-tail-call.ll:63
+  ret void
+}
----------------
rnk wrote:
> I'd throw in a test case where %x is a local alloca, since that should also work.
OK, that makes sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D52895