This is an archive of the discontinued LLVM Phabricator instance.

Inliner: Don't clear tail flags when creating allocas for byval args
ClosedPublic

Authored by rnk on Apr 16 2014, 2:59 PM.

Details

Summary

This is essentially a partial revert of r105255, which fixed PR7272.
The test case it introduced[1] still passes:

test/Transforms/Inline/2010-05-31-ByvalTailcall.ll

I think this was misdiagnosed. The call to @ext in the test case should
never have been marked 'tail' by instcombine, tailcallelim, or any other
optimization because it captures a byval argument, which is using memory
from the caller. That's against the LangRef rules for tail calls. It
looks like the optimization that added the tail flag has been fixed in
the meantime, because the test still passes.

Diff Detail

Event Timeline

The change looks fine, but can you add a positive test case?

rnk updated this revision to Unknown Object (????).Apr 18 2014, 3:37 PM
  • Add tests
  • Actually fix the latent bug in tailcallelim, it wasn't fixed yet
chandlerc accepted this revision.Apr 18 2014, 3:43 PM

Hah! Nice though. Nits only, LGTM to commit with these fixes.

test/Transforms/Inline/byval-tail-call.ll
4–5

Nice. If you're doing this much surgery here, I would rename it without the date so that people can find the test case more easily (byval_tail_call.ll or whatever).

test/Transforms/TailCallElim/basic.ll
149

missing :

rnk added inline comments.Apr 18 2014, 3:53 PM
test/Transforms/Inline/byval-tail-call.ll
4–5

I did. :)

test/Transforms/TailCallElim/basic.ll
149

woops, thanks!

rnk closed this revision.May 19 2014, 7:23 AM
rnk updated this revision to Diff 9540.

Closed by commit rL206789 (authored by @rnk).