[PATCH] D133860: StackProtector: enable tail call optimization even without musttail

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D133860#3798549 <https://reviews.llvm.org/D133860#3798549>, @taolq wrote:

> In D133860#3796726 <https://reviews.llvm.org/D133860#3796726>, @efriedma wrote:
>
>> Do we care that the backend might not choose to tail-call a function even if it's in tail position?  The "tail" marker is a hint; depending on attributes, calling convention, etc., the backend might not actually generate a tail call.
>
> I am not sure about this. Is that possible that a musttail-call is handled but a tail-call is not generated with all the same conditions?

Yes.  The most obvious case is if the caller has the "disable-tail-calls" attribute, but there are others.



================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:485
       Prev = Prev->getPrevNonDebugInstruction();
-      if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isMustTailCall())
+      if (Prev && isa<CallInst>(Prev) && cast<CallInst>(Prev)->isTailCall())
         CheckLoc = Prev;
----------------
taolq wrote:
> efriedma wrote:
> > Are you sure this actually does the right thing?  Consider something like:
> > 
> > ```
> > tail call void f()
> > call void g()
> > ret void
> > ```
> > 
> > If I'm following correctly, we'll insert the guard before the call to f(), which seems wrong.
> > 
> > (The old code worked because a musttail call is always in tail position; a "tail" call is not.)
> Thanks for your comment. I didn't notice that.
> So we should remove the latter if condition.
> Or let the verifier guarantee a tail call is always in the tail position.
> Which one could be better?
Other parts of CodeGen uses llvm::isInTailCallPosition to verify tail calls; I think it accepts more constructs than the verifier.

We intentionally allow calls marked "tail" in non-tail position: it simplifies the code that does the marking, and the "tail" marking can also be used for alias analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133860