Page MenuHomePhabricator

[CodeGen] Don't attempt a tail-call with a non-forwarded sret.
ClosedPublic

Authored by ab on Mar 20 2015, 7:08 PM.

Details

Reviewers
atrick
Summary

We used to do that, and it makes no sense. I haven't looked at other targets, but: ARM and X86 already disable tail-calls whenever there's sret (caller or callee, explicit or implicit), but AArch64 bravely tries to continue. In some cases, it generated broken code such as:

_test_tailcall_sret:
        sub     sp, sp, #128
        mov      x8, sp
        add     sp, sp, #128
        b       _test_sret

for:

declare i1024 @test_sret() #0
define i1024 @test_tailcall_sret() #0 {
  %a = tail call i1024 @test_sret()
  ret i1024 %a
}

There are two parts to this (I'll commit separately, but it makes sense to review together):

  • implicit sret: this will be part of the stack frame, so there's no way we can tail-call
  • explicit sret: a good enough approximation is: if the sret pointer is an Instruction, it might be function-local (alloca, usually).

In practice, both of these don't happen with well-behaved frontends such as clang, which will have an explicit sret, and forward it across tail calls.

Also, I say approximation because there's one case we pessimize (the GEP testcase), but that's a really weird situation.. Having a GetUnderlyingObject around the sret pointer origin check does the trick though, so I can add it if desired.

-Ahmed

Diff Detail

Event Timeline

ab updated this revision to Diff 22404.Mar 20 2015, 7:08 PM
ab retitled this revision from to [CodeGen] Don't attempt a tail-call with a non-forwarded sret..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added subscribers: Unknown Object (MLST), atrick, t.p.northover.

Ping!

-Ahmed

atrick accepted this revision.Mar 27 2015, 12:29 PM
atrick added a reviewer: atrick.

LGTM. I can't immediately think of a better hack than checking isa<Instruction>, and I can't think of a way to acquire a local stack address without an instruction.

This revision is now accepted and ready to land.Mar 27 2015, 12:29 PM
ab closed this revision.Apr 7 2015, 10:15 AM

This went in already, r233409/r233410.