This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Relax restriction on variadic functions for tailcall optimization
ClosedPublic

Authored by pbarrio on Nov 16 2016, 7:14 AM.

Details

Summary

Variadic functions can be treated in the same way as normal functions
with respect to the number and types of parameters.

Event Timeline

pbarrio updated this revision to Diff 78186.Nov 16 2016, 7:14 AM
pbarrio retitled this revision from to [ARM] Relax restriction on variadic functions for tailcall optimization.
pbarrio updated this object.
pbarrio added reviewers: rengolin, olista01.
pbarrio added a subscriber: llvm-commits.

Hello @rengolin , @olista01 ,

Could you have a look at this patch? I see no reason to restrict the tail call optimizations to variadic functions: the AAPCS restrictions seem to be already checked for non-variadic functions. The LLVM regression tests passed, and I have also tested with a few benchmarks and they all seem to work fine.

Please shout if I overlooked something. New test suggestions will be also very welcome.

Thanks,
Pablo

This was added by Jim in 2010, so I'm assuming it's something related to Darwin.

Adding Tim and Jim as reviewers.

t.p.northover edited edge metadata.Nov 16 2016, 7:50 AM

Jim's change seems to have been a whitespace tidy-up. It actually seems to hail from the very first tailcall commit in r105413 so it was probably just paranoia.

I can't think of any reason why it should be banned either so I say go for it.

olista01 edited edge metadata.Nov 16 2016, 7:57 AM

The change itself looks fine, I have a few suggestions for testing:

The test is checking the APCS ABI, could you add some extra RUN lines to check the newer AAPCS ABI? In particular, v_caller_ints2 will not be tail-callable in AAPCS because i64s must be in an even-odd register pair, so the last argument will go on the stack.

I'd also suggest adding some tests using doubles, and using the arm_aapcs_vfpcc calling convention, as doubles get treated differently for variadic and non-variadic calls.

Jim's change seems to have been a whitespace tidy-up.

Oh, apologies for the lack of thoroughness on my research. :)

pbarrio updated this revision to Diff 78243.Nov 16 2016, 12:06 PM
pbarrio edited edge metadata.

More testing: add tests for AAPCS and for floating-point arguments to variadic/non-variadic functions.

rengolin accepted this revision.Nov 16 2016, 12:48 PM
rengolin edited edge metadata.

Small comment on the tests, otherwise, LGTM. Thanks!

test/CodeGen/ARM/tail-call.ll
43

You can make all of them use --check-prefix=CHECK and then have only one CHECK-LABEL.

This revision is now accepted and ready to land.Nov 16 2016, 12:48 PM
pbarrio updated this revision to Diff 78339.Nov 17 2016, 2:59 AM
pbarrio edited edge metadata.

Cleaned up tests as per Renato's comments.

Thanks all for the reviews!

This revision was automatically updated to reflect the committed changes.

LGTM, thanks!

test/CodeGen/ARM/tail-call.ll
30

Heh, this was being completely ignored before. :)