Page MenuHomePhabricator

[X86] Add new calling convention that guarantees tail call optimization
ClosedPublic

Authored by dwightguth on Sep 20 2019, 1:08 PM.

Details

Summary

When the target option GuaranteedTailCallOpt is specified, calls with the fastcc calling convention will be transformed into tail calls if they are in tail position. This diff adds a new calling convention, tailcc, currently supported only on X86, which behaves the same way as fastcc, except that the GuaranteedTailCallOpt flag does not need to enabled in order to enable tail call optimization.

Diff Detail

Event Timeline

dwightguth created this revision.Sep 20 2019, 1:08 PM

I'd think this needs more test coverage.

llvm/test/CodeGen/X86/tailcall-tailcc.ll
1

Please use FileCheck + update_llc_test_checks.py

rnk added a comment.Sep 20 2019, 3:28 PM

I'd just like to note that this code is not well tested. A number of features and platforms have been added to the x86 prologue codepath that have not been tested in combination with guaranteed TCO, such as Win64 support. I've always assumed that users of this feature just like to live life dangerously.

Aside from that, the code looks correct, and I think this is a step towards simplifying things. As a long term goal, I think we should get rid of the GuaranteedTailCallOpt option and just ask people to use this convention. These are the conventions that work with this feature today:

  • fastcc, replaced by tailcc
  • GHC, we can probably make this always be callee cleanup and guarantee TCO for it in the same way
  • HiPE, probably the same
  • HHVM, unclear, I hope it's the same
  • X86_RegCall, I'm not sure if it was intended for us to change the ABI of this convention when users pass -tailcallopt to llc. It seems like that could be a bug.
llvm/lib/Target/X86/X86ISelLowering.cpp
3694–3695

There are four cases this handles:

  • regular calls
  • safe, friendly, good, ABI-preserving, opportunistic tail calls, called "sibcalls" in this code
  • musttail calls, which, because of the IR verifier checks, we know will not require moving the return address
  • ABI changing, return-address-moving, guaranteed tail calls

We have named booleans for case 2 and 3, but this code would benefit from a bool for case 4. I think you can call shouldGuaranteeTCO earlier and set up a bool with a name like GuaranteeTCO and check it in the rest of the cases throughout LowerCall.

llvm/test/CodeGen/X86/tailcall-tailcc.ll
1

+1

  • use FileCheck for test
  • refactor out named boolean variable
  • add more tests

A couple things I think could have testcases:

  • Mismatched calling conventions should not tail call
  • Interaction with musttail (see test/CodeGen/X86/musttail-fastcall.ll)
  • What should we do with disable-tail-calls? (see test/CodeGen/X86/disable-tail-calls.ll)
llvm/docs/LangRef.rst
439–448

Instead of copying the documentation from fastcc, it might be better to say this is equivalent (other than the tail call part)

dwightguth marked 4 inline comments as done.Sep 24 2019, 8:49 AM
dwightguth added inline comments.
llvm/docs/LangRef.rst
439–448

It's not totally equivalent though... it's only equivalent to fastcc if you pass -tailcallopt. I agree with what you're proposing at a high level, but I'm not sure how to word it. Do you have a suggestion?

paquette added inline comments.Sep 24 2019, 9:20 AM
llvm/docs/LangRef.rst
439–448

Maybe something like this?

"This calling convention ensures that calls in tail position will always be tail call optimized. This calling convention is equivalent to fastcc with an additional guarantee that tail calls will be produced whenever possible. Tail calls can only be optimized when this..."

add tests for mixed calling convention and musttail

I added tests for musttail and mixed calling conventions. But I'm a little unsure what the correct behavior should be here for disable-tail-calls. When you do a musttail call, disable-tail-calls is ignored. When you do a fastcc call with -tailcallopt, disable-tail-calls is not ignored. This seems a little inconsistent to me, but is beyond the scope of this revision. Which of these two behaviors should be implemented by tailcc, though? Currently it does the same thing as fastcc with -tailcallopt. I can add a test for that behavior, but if we want the other behavior, I need to know before I write that test because it affects the test's output.

Looking back in the discussion, I think it's fine to have it act the same way as fastcc.

If we want to get rid of GuaranteedTailCallOpt, I don't think it's a good idea to have tailcc disregard (or error out on) disable-tail-calls. We will still want to have the option to produce normal calls when someone asks for it.

I think it would be good to hear @rnk's thoughts on this too.

add test for disable-tail-calls

rnk added a comment.Sep 24 2019, 11:41 AM

I think of "disable-tail-calls" as a debugging tool, so I think what you've implemented here makes sense.

Updated documentation text in LangRef.rst

@rnk @paquette what does this need to move forward? I think I addressed all your comments.

rnk accepted this revision.Oct 7 2019, 1:43 PM

(back from vacation) Thanks, I think this looks good. Would you like somebody to commit this?

This revision is now accepted and ready to land.Oct 7 2019, 1:43 PM

It's ready on my end. I don't have commit access, so...

rnk added a comment.Oct 7 2019, 3:49 PM

It's ready on my end. I don't have commit access, so...

I committed this as rL373976, thanks!

I always ask permission to commit other folks patches just to be sure. We're not on github yet so we don't have a nice story for separating the patch author from the "approver" or "merger". Soon, though.

This revision was automatically updated to reflect the committed changes.