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

[dmgreen]

Summary

Adding NonNull as attributes to returned pointers has the unfortunate side effect of disabling tail calls. This
ignores the NonNull attribute, in the same way that we ignore the NoAlias attribute as it has no affect on the
calling convention.

[efriedma]

lib/CodeGen/CodeGenPrepare.cpp

1876 ↗

(On Diff #165987)

I don't follow why you're deleting this code.

[dmgreen]

lib/CodeGen/CodeGenPrepare.cpp

1876 ↗

(On Diff #165987)

Oh right, I meant to put a comment about that. From what I can tell it appears to be testing CalleeAttrs against itself, and this check should be handled by the calls to attributesPermitTailCall above. Unless I'm missing what this is testing?

[efriedma]

lib/CodeGen/CodeGenPrepare.cpp

1876 ↗

(On Diff #165987)

Oh, yes, I see. This isn't doing anything.

Please make sure we have appropriate test coverage (maybe in test/CodeGen/X86/tailcall-cgp-dup.ll).

lib/CodeGen/SelectionDAG/TargetLowering.cpp

63 ↗

(On Diff #165987)

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.)

[dmgreen]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

63 ↗

(On Diff #165987)

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.

[dmgreen]

Added a tailcall duplication opt test to Transforms/CodeGenPrepare.

[dmgreen]

Minor edit to test (and renamed it to tailcall-dup.ll).

[efriedma]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

63 ↗

(On Diff #165987)

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.

[dmgreen]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

63 ↗

(On Diff #165987)

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.

[efriedma]

LGTM with a minor tweak.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

63 ↗

(On Diff #165987)

Weird... it's probably not really checking what it's supposed to.

Anyway, not really worth worrying about.

test/CodeGen/ARM/tail-call.ll

102 ↗

(On Diff #166094)

Wrong comment?

[dmgreen]

Thanks!

llvm/trunk/lib/CodeGen/Analysis.cpp

bool llvm::attributesPermitTailCall(const Function *F, const Instruction *I,
   bool DummyADS;
   bool &ADS = AllowDifferingSizes ? *AllowDifferingSizes : DummyADS;
   ADS = true;
 
   AttrBuilder CallerAttrs(F->getAttributes(), AttributeList::ReturnIndex);
   AttrBuilder CalleeAttrs(cast<CallInst>(I)->getAttributes(),
                           AttributeList::ReturnIndex);
 
-  // Noalias is completely benign as far as calling convention goes, it
-  // shouldn't affect whether the call is a tail call.
+  // NoAlias and NonNull are completely benign as far as calling convention
+  // goes, they shouldn't affect whether the call is a tail call.
   CallerAttrs.removeAttribute(Attribute::NoAlias);
   CalleeAttrs.removeAttribute(Attribute::NoAlias);
+  CallerAttrs.removeAttribute(Attribute::NonNull);
+  CalleeAttrs.removeAttribute(Attribute::NonNull);
 
   if (CallerAttrs.contains(Attribute::ZExt)) {
     if (!CalleeAttrs.contains(Attribute::ZExt))
       return false;
 
     ADS = false;
     CallerAttrs.removeAttribute(Attribute::ZExt);
     CalleeAttrs.removeAttribute(Attribute::ZExt);

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

if (PN) {
     }
   }
 
   bool Changed = false;
   for (unsigned i = 0, e = TailCalls.size(); i != e; ++i) {
     CallInst *CI = TailCalls[i];
     CallSite CS(CI);
 
-    // Conservatively require the attributes of the call to match those of the
-    // return. Ignore noalias because it doesn't affect the call sequence.
-    AttributeList CalleeAttrs = CS.getAttributes();
-    if (AttrBuilder(CalleeAttrs, AttributeList::ReturnIndex)
-            .removeAttribute(Attribute::NoAlias) !=
-        AttrBuilder(CalleeAttrs, AttributeList::ReturnIndex)
-            .removeAttribute(Attribute::NoAlias))
-      continue;
-
     // Make sure the call instruction is followed by an unconditional branch to
     // the return block.
     BasicBlock *CallBB = CI->getParent();
     BranchInst *BI = dyn_cast<BranchInst>(CallBB->getTerminator());
     if (!BI || !BI->isUnconditional() || BI->getSuccessor(0) != BB)
       continue;
 
     // Duplicate the return into CallBB.

llvm/trunk/lib/CodeGen/SelectionDAG/TargetLowering.cpp

 
 /// Check whether a given call node is in tail position within its function. If
 /// so, it sets Chain to the input chain of the tail call.
 bool TargetLowering::isInTailCallPosition(SelectionDAG &DAG, SDNode *Node,
                                           SDValue &Chain) const {
   const Function &F = DAG.getMachineFunction().getFunction();
 
   // Conservatively require the attributes of the call to match those of
-  // the return. Ignore noalias because it doesn't affect the call sequence.
+  // the return. Ignore NoAlias and NonNull because they don't affect the
+  // call sequence.
   AttributeList CallerAttrs = F.getAttributes();
   if (AttrBuilder(CallerAttrs, AttributeList::ReturnIndex)
           .removeAttribute(Attribute::NoAlias)
+          .removeAttribute(Attribute::NonNull)
           .hasAttributes())
     return false;
 
   // It's not safe to eliminate the sign / zero extension of the return value.
   if (CallerAttrs.hasAttribute(AttributeList::ReturnIndex, Attribute::ZExt) ||
       CallerAttrs.hasAttribute(AttributeList::ReturnIndex, Attribute::SExt))
     return false;
 

llvm/trunk/test/CodeGen/ARM/tail-call.ll

 ; e.g. one 32-bit integer, two 64-bit integers
 define void @v_caller_ints_fail(i32 %y, i64 %z) {
 ; CHECK-LABEL: v_caller_ints_fail:
 ; CHECK: bl variadic
 entry:
   %call = tail call i32 (i32, ...) @variadic(i32 %y, i64 %z, i64 %z)
   ret void
 }
+
+; Check that NonNull attributes don't inhibit tailcalls.
+
+declare nonnull i8* @nonnull_callee(i8* %p, i32 %val)
+define i8* @nonnull_caller(i8* %p, i32 %val) {
+; CHECK-LABEL: nonnull_caller:
+; CHECK-TAIL: b nonnull_callee
+; CHECK-NO-TAIL: bl nonnull_callee
+entry:
+  %call = tail call i8* @nonnull_callee(i8* %p, i32 %val)
+  ret i8* %call
+}

llvm/trunk/test/Transforms/CodeGenPrepare/ARM/tailcall-dup.ll

+; RUN: opt -codegenprepare -S < %s | FileCheck %s
+
+target triple = "armv8m.main-none-eabi"
+
+declare i8* @f0()
+declare i8* @f1()
+
+define i8* @tail_dup() {
+; CHECK-LABEL: tail_dup
+; CHECK: tail call i8* @f0()
+; CHECK-NEXT: ret i8*
+; CHECK: tail call i8* @f1()
+; CHECK-NEXT: ret i8*
+bb0:
+  %tmp0 = tail call i8* @f0()
+  br label %return
+bb1:
+  %tmp1 = tail call i8* @f1()
+  br label %return
+return:
+  %retval = phi i8* [ %tmp0, %bb0 ], [ %tmp1, %bb1 ]
+  ret i8* %retval
+}
+
+define nonnull i8* @nonnull_dup() {
+; CHECK-LABEL: nonnull_dup
+; CHECK: tail call i8* @f0()
+; CHECK-NEXT: ret i8*
+; CHECK: tail call i8* @f1()
+; CHECK-NEXT: ret i8*
+bb0:
+  %tmp0 = tail call i8* @f0()
+  br label %return
+bb1:
+  %tmp1 = tail call i8* @f1()
+  br label %return
+return:
+  %retval = phi i8* [ %tmp0, %bb0 ], [ %tmp1, %bb1 ]
+  ret i8* %retval
+}
+
+define i8* @noalias_dup() {
+; CHECK-LABEL: noalias_dup
+; CHECK: tail call noalias i8* @f0()
+; CHECK-NEXT: ret i8*
+; CHECK: tail call noalias i8* @f1()
+; CHECK-NEXT: ret i8*
+bb0:
+  %tmp0 = tail call noalias i8* @f0()
+  br label %return
+bb1:
+  %tmp1 = tail call noalias i8* @f1()
+  br label %return
+return:
+  %retval = phi i8* [ %tmp0, %bb0 ], [ %tmp1, %bb1 ]
+  ret i8* %retval
+}
+
+; Use inreg as a way of testing that attributes (other than nonnull and
+; noalias) disable the tailcall duplication in cgp.
+
+define inreg i8* @inreg_nodup() {
+; CHECK-LABEL: inreg_nodup
+; CHECK: tail call i8* @f0()
+; CHECK-NEXT: br label %return
+; CHECK: tail call i8* @f1()
+; CHECK-NEXT: br label %return
+bb0:
+  %tmp0 = tail call i8* @f0()
+  br label %return
+bb1:
+  %tmp1 = tail call i8* @f1()
+  br label %return
+return:
+  %retval = phi i8* [ %tmp0, %bb0 ], [ %tmp1, %bb1 ]
+  ret i8* %retval
+}