Page MenuHomePhabricator

Add support for function attribute "notail"
ClosedPublic

Authored by ahatanak on Sep 16 2015, 7:21 PM.

Details

Summary

This patch adds support for a new function attribute "notail". The attribute is used to disable tail call optimization on calls to functions marked with the attribute.

This is different from the attribute proposed in D12547, which disables tail call optimizations on call sites within the marked function.

http://reviews.llvm.org/D12547

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 34960.Sep 16 2015, 7:21 PM
ahatanak retitled this revision from to Add support for function attribute "notail".
ahatanak updated this object.
ahatanak added a reviewer: aaron.ballman.
ahatanak added a subscriber: cfe-commits.

I wonder if 'notailcall' be better, since it contains a verb (like 'noinline'). I don't have a strong opinion; just putting the alternative out there.

ahatanak updated this revision to Diff 37172.Oct 12 2015, 3:06 PM

I've made some changes following the discussion I had and the feedback I got on the llvm-side patch.

This is the link to the discussion thread:
http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/271105/

The difference between the previous patch and this patch lies in the way indirect calls (at the source level) are handled. The approach taken in the previous patch attached the notail attribute to the declaration or definition of functions in the IR, which enabled blocking tail call optimization on an indirect call site if the compiler could statically determine it was calling a function marked notail. In this patch, clang marks a call instruction in the IR as notail only if it is a direct call to a function marked "notail". It does nothing to prevent tail call on indirect call sites.

I think there are a couple of things that have to be discussed:

  1. Name of the attribute: Should it be "notail" or "notailcall"? Perhaps it should be named something like "nodirecttail" to indicate it is used to block direct calls, but not indirect calls?
  1. Can we guarantee or promise that the attribute will *always* block direct calls (but do nothing for indirect calls)? Or should we say this attribute only prevents tail call (even for direct calls) on a best effort basis in case it turns out there are cases where it isn't possible to keep that promise?
aaron.ballman edited edge metadata.Oct 13 2015, 9:55 AM

I think there are a couple of things that have to be discussed:

  1. Name of the attribute: Should it be "notail" or "notailcall"? Perhaps it should be named something like "nodirecttail" to indicate it is used to block direct calls, but not indirect calls?

The attribute prevents tail calls from the call site, not for the function itself. "notail", at least to me, implies that the function definition itself will not have TCO; it does not imply that callers will be affected. Perhaps "not_tail_called" or "disable_tail_calls"? It's a hard concept to get across in a small identifier.

Also, I would like to see a test that shows always_inline, notail also gets diagnosed as mutually exclusive, and a test that the right thing happens with a C++ spelling on a member function.

~Aaron

include/clang/Basic/AttrDocs.td
1619 ↗(On Diff #37172)

It would be good to clarify what you mean by direct calls, as not all of the readers are going to understand that easily. Perhaps a simple code example showing a call to the function, and a call to the same function through a function pointer, with comments showing which one has TCO.

You should also call out that it is mutually exclusive with always_inline.

lib/Sema/SemaDeclAttr.cpp
1705 ↗(On Diff #37172)

Missing the same check in the always_inline attribute handler.

ahatanak updated this revision to Diff 37437.Oct 14 2015, 7:17 PM
ahatanak edited edge metadata.

Address some of the review comments:

  • Added a c++ test which tests the c++ spelling of the attribute on member functions and shows which virtual functions calls get marked as "notail" in the IR.
  • Added a check that was missing in always_inline attribute handler.
  • Fixed test/Sema/attr-notail.c to test always_inline and notail are mutually exclusive, regardless of the order in which they are added to the function.
ahatanak marked an inline comment as done.Oct 14 2015, 7:26 PM
ahatanak added inline comments.
include/clang/Basic/AttrDocs.td
1619 ↗(On Diff #37437)

Perhaps direct call isn't the right term to use for this attribute. I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function.

ahatanak updated this revision to Diff 37816.Oct 19 2015, 5:59 PM

Address review comments:

  1. Renamed the attribute to "not_tail_called".

I chose "not_tail_called" over "notailcall" or "notail" to better distinguish it from the attribute that is proposed in http://reviews.llvm.org/D12547 (which is tentatively named "disable_tail_calls").

  1. Made changes to error-out if a virtual function or an objective-c method is marked "not_tail_called".

I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have.

  1. Added code examples to AttrDocs.td.
aaron.ballman added inline comments.Nov 1 2015, 5:32 PM
lib/Sema/SemaDecl.cpp
5374 ↗(On Diff #37816)

Is there a reason this is here instead of SemaDeclAttr.cpp? It seems like the decl passed in with the attribute attached to it should already be known to be virtual or not?

test/Sema/attr-notail.c
8 ↗(On Diff #37816)

Missing a test case for the attribute being specified on something other than a function, and one for being passed arguments.

test/SemaCXX/attr-notail.cpp
5 ↗(On Diff #37816)

Missing the "s" on virtual functions.

ahatanak added inline comments.Nov 1 2015, 11:28 PM
lib/Sema/SemaDecl.cpp
5374 ↗(On Diff #37816)

I initially did the check in SemaDeclAttr.cpp but found out that methods declared override are not known to be virtual (isVirtual returns false) when ProcessDeclAttributes is called:

class Base {

// This method is fine.
virtual int foo1();

};

class Derived1 : public Base {

// isVirtual() is false for the declaration of this method.
int foo1() override;

};

This is because ProcessDeclAttributes is called before CheckFunctionDeclaration (in line 7952, which calls Sema::AddOverriddenMethods) is called in Sema::ActOnFunctionDeclarator. Unless there is another way to check if a method is virtual, I don't think we can move the check to SemaDeclAttr.cpp?

ahatanak updated this revision to Diff 38967.Nov 2 2015, 12:30 PM

Fixed test cases:

  • Added a test case for the attribute being specified on a variable.
  • Added a missing 's' to "virtual function" in the check strings.
ahatanak added inline comments.Nov 2 2015, 12:32 PM
test/Sema/attr-notail.c
9 ↗(On Diff #38967)

I didn't understand what kind of test case is required for not_tail_called on passed arguments. Could you give me an example?

aaron.ballman added inline comments.Nov 2 2015, 4:14 PM
test/Sema/attr-notail.c
9 ↗(On Diff #38967)

Sorry, I may have been unclear. I meant the attribute being passed arguments itself. e.g.,

int f() __attribute__((not_tail_called("this should diagnose")));
ahatanak updated this revision to Diff 39003.Nov 2 2015, 4:30 PM

Added test case that produces an error diagnostic if an argument is passed for not_tail_called.

aaron.ballman added inline comments.Nov 2 2015, 4:39 PM
lib/Sema/SemaDecl.cpp
5374 ↗(On Diff #39003)

I am not home yet, and so I don't have the source code to try this out, but I have a sneaking suspicion there's a way to tell whether a function is an override. IIRC, it's something like overridden_methods() on a CXXMethodDecl.

I will do some poking when I am back in front of the source. Otherwise, I would guess you can look at some of the diagnostics in DiagnosticSemaKinds.td for the override keyword itself, because I bet we warn when you specify override on something that isn't a virtual function override, and that should have a code example.

ahatanak added inline comments.Nov 2 2015, 10:10 PM
lib/Sema/SemaDecl.cpp
5374 ↗(On Diff #39003)

It's possible to tell a method is a virtual function if either an overridden method is added to its declaration (which will cause isVirtual() to return true) or an OverrideAttr attribute is added. Currently, processDeclAttributes is called before these methods or attributes are added, and that is why I think it isn't possible to tell if the method is virtual when the attributes are being checked.

aaron.ballman accepted this revision.Nov 4 2015, 9:16 AM
aaron.ballman edited edge metadata.

LGTM!

lib/Sema/SemaDecl.cpp
5374 ↗(On Diff #39003)

Ugh, that is really unfortunate, but I think you're right. At some point, we may have to find a way to handle this, but I don't think that should hold up your patch.

This revision is now accepted and ready to land.Nov 4 2015, 9:16 AM

Thanks, I'll see how the review for the llvm-side patch goes and commit both patches after it is approved.

This revision was automatically updated to reflect the committed changes.