Page MenuHomePhabricator

[DAE] don't remove args of musttail target/caller
ClosedPublic

Authored by indutny on Feb 23 2018, 6:02 PM.

Details

Summary

musttail requires identical signatures of caller and callee. Removing
arguments breaks musttail semantics.

PR36441

Diff Detail

Repository
rL LLVM

Event Timeline

indutny created this revision.Feb 23 2018, 6:02 PM

A question: what is backport policy of LLVM project? I'd love to see this fix in 5.0.0 .

indutny updated this revision to Diff 135768.Feb 23 2018, 7:39 PM

Use getTerminatingMustTailCall instead of iterating through all instructions.

I'm not sure I know enough about the subject matter to give more than a superficial review.

lib/Transforms/IPO/DeadArgumentElimination.cpp
512 ↗(On Diff #135768)

range-based-for?

test/Transforms/DeadArgElim/musttail-caller.ll
2 ↗(On Diff #135768)

It would be better to write a comment here explaining what behavior is being tested instead of just referring to the PR>.

indutny added inline comments.Feb 26 2018, 12:04 PM
lib/Transforms/IPO/DeadArgumentElimination.cpp
512 ↗(On Diff #135768)

FWIW, I've just added the braces to the existing code. It seemed to be out-of-scope change when I was working on it. Should I do it now?

indutny updated this revision to Diff 135945.Feb 26 2018, 12:08 PM

Added explanation to the test.

indutny marked an inline comment as done.Feb 26 2018, 12:09 PM
rnk added a subscriber: rnk.Feb 26 2018, 4:19 PM

Thanks for the fix!

test/Transforms/DeadArgElim/musttail-caller.ll
6 ↗(On Diff #135945)

Do we pass undef to the call? That would be correct. It doesn't matter what we pass, so long as we match the prototype.

rnk added a reviewer: rnk.Feb 26 2018, 4:19 PM
indutny added inline comments.Feb 26 2018, 5:26 PM
test/Transforms/DeadArgElim/musttail-caller.ll
6 ↗(On Diff #135945)

I'm not sure I got your comment. Do you want me to pass undef below?

rnk added inline comments.Feb 26 2018, 5:32 PM
test/Transforms/DeadArgElim/musttail-caller.ll
6 ↗(On Diff #135945)

No. Can you add CHECKs for the musttail call to show what arguments are passed here? I'm wondering if we get or miss that optimization. It is invalid to change the prototype of the foo, but it is valid to pass undef because the args are dead.

indutny added inline comments.Feb 26 2018, 5:52 PM
test/Transforms/DeadArgElim/musttail-caller.ll
6 ↗(On Diff #135945)

I believe it is musttail call void @foo(i32 %a, i32 0)

indutny updated this revision to Diff 136018.Feb 26 2018, 5:54 PM

Add extra check to the test.

rnk accepted this revision.Feb 27 2018, 10:34 AM

lgtm with nit

test/Transforms/DeadArgElim/musttail-caller.ll
6 ↗(On Diff #135945)

Can you add a FIXME that we should replace these with undef?

This revision is now accepted and ready to land.Feb 27 2018, 10:34 AM
indutny updated this revision to Diff 136105.Feb 27 2018, 10:42 AM
indutny marked 7 inline comments as done.

Add FIXME.

Thank you for review!

@rnk not sure if it matters, but I don't have commit access yet. If you could land the change - I'd appreciate it a lot. Thank you!

This revision was automatically updated to reflect the committed changes.

Thank you, everyone!