This is an archive of the discontinued LLVM Phabricator instance.

Add support for function attribute "disable_tail_calls"
ClosedPublic

Authored by ahatanak on Sep 1 2015, 5:16 PM.

Details

Summary

There have been requests for a function attribute that disables tail call optimizations in the backend. This patch defines such an attribute.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 33762.Sep 1 2015, 5:16 PM
ahatanak retitled this revision from to Add support for function attribute "disable_tail_calls".
ahatanak updated this object.
ahatanak added a subscriber: cfe-commits.
aaron.ballman added inline comments.Sep 2 2015, 7:20 AM
include/clang/Basic/Attr.td
894

We already have an attribute that is tangentially related: OptimizeNone. Would it make sense to have that attribute take arguments that control what optimizations are enabled/disabled? How should these two attributes (and the resulting functionality) interact? For instance, if I specify optnone on a function, should that also disable tail call optimizations by creating an implicit instance of this new attribute?

895

Do we also want a C++ spelling for this attribute, under the clang namespace?

896

Should this also apply to Objective-C methods?

897

Please document this attribute.

lib/CodeGen/CGCall.cpp
1481–1482

Better to move this into the block that already tests TargetDecl (around line 1403 or so).

1484

Can do this in a single line with ?:

lib/Sema/SemaDeclAttr.cpp
4882

Are there semantic checks we would like to perform on the declaration for nonsense uses of this attribute, such as functions marked [[noreturn]], etc?

ahatanak updated this revision to Diff 33882.Sep 2 2015, 4:40 PM
ahatanak added inline comments.Sep 2 2015, 4:43 PM
include/clang/Basic/Attr.td
894

optnone (or -O0) doesn't disable tail call optimization if clang attaches "tail" to a call instruction (see lib/CodeGen/CGObjC.cpp for examples). I think tail call optimization should be disabled even for functions marked as "tail" by clang if the user adds an attribute to the function in the source code with the intention of disabling tail call optimization. For that reason, I don't think we can use OptimizeNone for this purpose.

895

Done.

896

Done.

897

Done.

lib/CodeGen/CGCall.cpp
1481–1482

It looks like moving this code into the block would make the code mode complex, so I followed your suggestion below.

1484

I defined a boolean variable and passed it to llvm::toStringRef to avoid calling addAttribute in two places.

lib/Sema/SemaDeclAttr.cpp
4882

Some of the users asked for this function attribute to ensure stack frames are fully preserved for backtrace, so it's entirely reasonable for a user to mark a function "noreturn" that calls a non-returning function and at the same time expect the backend to avoid tail call optimization. I'm thinking we shouldn't prohibit attaching "noreturn" and "disable_tail_calls" to the same function. What do you think?

Other than "noreturn", I didn't find any attributes that shouldn't be used with "disable_tail_calls" in the following link:

http://clang.llvm.org/docs/AttributeReference.html

aaron.ballman added inline comments.Sep 3 2015, 6:48 AM
include/clang/Basic/Attr.td
894

Pardon me if this is obvious, but -- are there times when you would want to disable tail calls but leave other optimizations in place? I'm trying to understand why these attributes are orthogonal.

896

Should this attribute be plural? Are there multiple tail calls within a single method that can be optimized away?

include/clang/Basic/AttrDocs.td
1619

I would avoid using the exact syntax here because this is a GNU and C++ attribute. Could just say:

The `disable_tail_calls` attribute instructs the backend to not perform tail call optimization.

lib/Sema/SemaDeclAttr.cpp
4882

Okay, that makes sense. I can contrive examples of noreturn where TCO could happen, it just took me a while to think of them. ;-)

What about other semantic checks, like warning the user about disabling TCO when TCO could never be enabled in the first place? Can you disable TCO for functions that are marked attribute((naked))? What about returns_twice?

Unfortunately, we have a lot of attributes for which we've yet to write documentation, so you may need to look through Attr.td.

aaron.ballman edited edge metadata.Sep 3 2015, 6:50 AM
aaron.ballman added a subscriber: aaron.ballman.

Oops, apologies for making this harder than it needs to be. It seems
phab didn't provide context to these comments. Check out the phab
review link for more context, if you'd like it.

~Aaron

ahatanak updated this revision to Diff 34872.Sep 15 2015, 9:46 PM
ahatanak edited edge metadata.

Sorry for the delay in my response.

I had discussions with the users who requested this feature and it turns out they were asking for two different kinds of attributes. They are both needed to disable tail call optimization to preserve the stack frame but they differ in which stack frame they are trying to preserve. The attribute in this patch disables optimization on the call sites inside a function to preserve the stack frame of the function. The other attribute disables tail call *to* the function and therefore preserves the stack frame of the calling function.

I'll send a patch for the other attribute shortly.

include/clang/Basic/Attr.td
894

Yes, that's correct. The new function attribute we are adding shouldn't disable other optimizations that are normally run at -O2 or -O3.

896

I named it after the existing IR function attribute and I'm guessing the plural name was chosen because functions can have multiple tail call sites. I think the singular name is better if we want this attribute to mean "disable tail call optimization".

include/clang/Basic/AttrDocs.td
1619

OK, done.

lib/Sema/SemaDeclAttr.cpp
4882

Since "naked" allows only inline assembly statements, it should be an error to have disable-tail-calls and naked on the same function. I made changes in Sema to detect that case.

My understanding is that you can't do tail call optimization on call sites of a function that calls a "return_twice" function. However, attaching "return_twice" on the calling function doesn't block tail call optimizations on the call sites inside the function.

I didn't find any other attributes that seemed incompatible with disable-tail-calls.

ahatanak updated this revision to Diff 34919.Sep 16 2015, 1:30 PM

Made the following changes to address review comments:

  • Use checkAttrMutualExclusion.
  • Removed spaces in the check strings in test case attr-disable-tail-calls.c.

Are you still looking for review on this patch, or are you intending to make modifications based on further discussion from the other tail call attribute?

I intend to change the documentation, but other than that there should be no changes.

I'll upload a rebased patch after I commit the other tail call patches.

ahatanak updated this revision to Diff 39702.Nov 9 2015, 8:41 AM

I ended up making a few more changes based on the feedback on the not_tail_called attribute:

  • Changed wording in docs and added a code example.
  • Made changes to detect both (naked,disable_tail_calls) and (disable_tail_calls,naked) on a function.
  • Added test cases that check disable_tail_calls is not on variables and doesn't take arguments.

Missing tests demonstrating use of the C++ spelling of the attribute. Perhaps a test showing it on a member function would be useful.

Out of curiosity, what would be the expected behavior of the following:

struct B {
  int g(int);
  [[clang::disable_tail_calls]] virtual int f(int a); // is this okay?
};

struct D : B {
  int f(int a) override { // is this okay?
    return g(a); // Does this get TCO?
  }
};

void f() {
  B *b = new D;
  b->f(12);
}
include/clang/Basic/AttrDocs.td
1626

It would be useful to have a declaration for callee() to make it obvious that it *could* have been TCOed otherwise.

lib/CodeGen/CGCall.cpp
1479

I would swap the order of the checks. It's less expensive when CodeGenOpts.DisableTailCalls is true than it is to call hasAttr().

Marking virtual functions as disable_tail_calls is fine since disable_tail_calls affects the call sites inside the body of the marked function. In your example, it prevents tail call optimization on call sites inside B::g, but doesn't affect call sites in D::g.

Marking virtual functions as disable_tail_calls is fine since disable_tail_calls affects the call sites inside the body of the marked function. In your example, it prevents tail call optimization on call sites inside B::g, but doesn't affect call sites in D::g.

Okay, that's reasonable, but you should definitely update the documentation to reflect that and have at least one codegen test that ensures the behavior is implemented as-expected.

ahatanak updated this revision to Diff 40011.Nov 11 2015, 10:22 PM

Address review comments.

aaron.ballman added inline comments.Nov 12 2015, 6:31 AM
test/SemaCXX/attr-disable-tail-calls.cpp
2 ↗(On Diff #40011)

Is this file missing an expected-no-diagnostics?

ahatanak updated this revision to Diff 40060.Nov 12 2015, 9:10 AM
ahatanak marked 2 inline comments as done.

Added "expected-no-diagnostics" to test case.

aaron.ballman accepted this revision.Nov 12 2015, 12:16 PM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Nov 12 2015, 12:16 PM
ahatanak marked an inline comment as done.Nov 12 2015, 1:06 PM

I'll commit this patch shortly. Thank you for the review.

This revision was automatically updated to reflect the committed changes.