[PATCH] D52238: [CodeGen] Enable tail calls for functions with NonNull attributes.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM with a minor tweak.



================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:63
           .removeAttribute(Attribute::NoAlias)
+          .removeAttribute(Attribute::NonNull)
           .hasAttributes())
----------------
dmgreen wrote:
> efriedma wrote:
> > dmgreen wrote:
> > > efriedma wrote:
> > > > Test coverage for this change?  (This codepath is primarily used by SelectionDAGLegalize::ExpandLibCall, so probably unlikely to be relevant in practice, but it should be possible to construct a synthetic testcase.)
> > > My understanding is that we need an intrinsic that returns a ptr and is lowered to a call. Any idea what that might be? I had a look round and couldn't find anything, and an assert that RetTy isn't a pointer type didn't throw up anything in the test suite.
> > You don't actually need an intrinsic that returns pointer; you can just take an intrinsic that returns an int and use inttoptr on the result.
> The check in ExpandLibcall seems to be:
>    bool isTailCall = 
>       TLI.isInTailCallPosition(DAG, Node, TCChain) &&
>       (RetTy == F.getReturnType() || F.getReturnType()->isVoidTy());
> If I comment out that return type check, it does seem to tail call something like __clzsi2 as expected (and nonnull used to disable this, no longer does). But as-is, doesn't seem to like tail calling if the types don't match up.
Weird... it's probably not really checking what it's supposed to.

Anyway, not really worth worrying about.


================
Comment at: test/CodeGen/ARM/tail-call.ll:102
+
+; Check that NonNull attributes don't inhibit inlining.
+
----------------
Wrong comment?


https://reviews.llvm.org/D52238