Page MenuHomePhabricator

[CodeGen] skip lifetime end marker in isInTailCallPosition
ClosedPublic

Authored by rob.lougher on Oct 22 2018, 11:57 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

rob.lougher created this revision.Oct 22 2018, 11:57 AM
rnk added a comment.Oct 23 2018, 10:22 AM

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

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!

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.

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 accepted this revision.Oct 23 2018, 3:05 PM

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

This revision is now accepted and ready to land.Oct 23 2018, 3:05 PM
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.

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

This revision was automatically updated to reflect the committed changes.