[CodeGen] skip lifetime end marker in isInTailCallPosition

[rob.lougher]

Summary

Consider the following program:

------------- test.c ---------------
extern void bar(void);

__attribute__((__noinline__))
void foo(int *p) {
  *p = 10;
}

void foobar() {
  int i;
  foo(&i);
  bar();
}
------------------------------------

TailCallElim will correctly identify the call to bar as a tail call (as foo is nocapture). However, it will not be tail call optimized as it fails isInTailCallPosition() due to the lifetime end marker for the variable i:

define void @foobar() {
entry:
  %i = alloca i32, align 4
  %0 = bitcast i32* %i to i8*
  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0)
  call void @foo(i32* nonnull %i)
  tail call void @bar()
  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0)
  ret void
}

On x86_64 this generates:

foobar:
	pushq	%rax
	leaq	4(%rsp), %rdi
	callq	foo
	callq	bar
	popq	%rax
	retq

After this patch we get:

foobar:
	pushq	%rax
	leaq	4(%rsp), %rdi
	callq	foo
	popq	%rax
	jmp	bar                     # TAILCALL

[rnk]

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

[rob.lougher]

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

Good point. I couldn't find anything when I looked. Skipping of intrinsics seems to be done in an ad-hoc way throughout the code. Checks for debug intrinsics are common. The stack protector initially just skipped debug intrinsics - lifetime markers were added recently in D45331. The loop vectorizer checks for assume, lifetime_end, lifetime_start and sideeffect.

I only checked for lifetime_end because this is the only one which is likely to appear between a valid tail call and the ret. However, I can add assume, start and sideeffect? Thanks!

[probinson]

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I have a memory that somebody wanted an expands-to-nothing property, or generates-no-instructions, or something along those lines. That's probably the kind of generic property you're asking about here? I don't remember the context or outcome, unfortunately.

[rob.lougher]

In D53519#1273062, @probinson wrote:

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I have a memory that somebody wanted an expands-to-nothing property, or generates-no-instructions, or something along those lines. That's probably the kind of generic property you're asking about here? I don't remember the context or outcome, unfortunately.

See my previous comment about ad hoc checks of intrinsic IDs in stack protector, loop vectorizer, etc.

[rnk]

I'd just land it as is. I think you're right, there's no reason to extend this with a local ad-hoc list, since those other two known intrinsics are unlikely to appear here.f

[rob.lougher]

In D53519#1273358, @rob.lougher wrote:

In D53519#1273062, @probinson wrote:

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I have a memory that somebody wanted an expands-to-nothing property, or generates-no-instructions, or something along those lines. That's probably the kind of generic property you're asking about here? I don't remember the context or outcome, unfortunately.

See my previous comment about ad hoc checks of intrinsic IDs in stack protector, loop vectorizer, etc.

I can find no such property, and code which wants to skip such intrinsics uses explicit intrinsic ID checks, which again implies no such property exists. Debug intrinsics are commonly skipped (as they don't generate code). Other non-code intrinsics have been added as needed/discovered as in D45331. Like the stack protector, isInTailCallPosition has a check for debug intrinsics. This patch simply adds an additional intrinsic check for lifetime_end in the same manner.

[rob.lougher]

In D53519#1273387, @rnk wrote:

I'd just land it as is. I think you're right, there's no reason to extend this with a local ad-hoc list, since those other two known intrinsics are unlikely to appear here.f

Thanks! An interesting point is whether the stack protector should check for those intrinsics (I think probably).

lib/CodeGen/Analysis.cpp

bool llvm::isInTailCallPosition(ImmutableCallSite CS, const TargetMachine &TM) {
   if (I->mayHaveSideEffects() || I->mayReadFromMemory() ||
       !isSafeToSpeculativelyExecute(I))
     for (BasicBlock::const_iterator BBI = std::prev(ExitBB->end(), 2);; --BBI) {
       if (&*BBI == I)
         break;
       // Debug info intrinsics do not get in the way of tail call optimization.
       if (isa<DbgInfoIntrinsic>(BBI))
         continue;
+      // A lifetime end intrinsic should not stop tail call optimization.
+      if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI))
+        if (II->getIntrinsicID() == Intrinsic::lifetime_end)
+          continue;
       if (BBI->mayHaveSideEffects() || BBI->mayReadFromMemory() ||
           !isSafeToSpeculativelyExecute(&*BBI))
         return false;
     }
 
   const Function *F = ExitBB->getParent();
   return returnTypeIsEligibleForTailCall(
       F, I, Ret, *TM.getSubtargetImpl(*F)->getTargetLowering());

test/CodeGen/X86/tailcall-lifetime-end.ll

+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+
+; A lifetime end intrinsic should not prevent a call from being tail call
+; optimized.
+
+define void @foobar() {
+; CHECK-LABEL: foobar
+; CHECK: pushq  %rax
+; CHECK: leaq 4(%rsp), %rdi
+; CHECK: callq  foo
+; CHECK: popq %rax
+; CHECK: jmp  bar
+entry:
+  %i = alloca i32
+  %0 = bitcast i32* %i to i8*
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0)
+  call void @foo(i32* nonnull %i)
+  tail call void @bar()
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0)
+  ret void
+}
+
+declare void @foo(i32* nocapture %p)
+declare void @bar()
+
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)