Add support for function attribute "disable_tail_calls"

[ahatanak]

Summary

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

[aaron.ballman]

include/clang/Basic/Attr.td

894 ↗

(On Diff #33762)

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 ↗

(On Diff #33762)

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

896 ↗

(On Diff #33762)

Should this also apply to Objective-C methods?

897 ↗

(On Diff #33762)

Please document this attribute.

lib/CodeGen/CGCall.cpp

1477 ↗

(On Diff #33762)

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

1480 ↗

(On Diff #33762)

Can do this in a single line with ?:

lib/Sema/SemaDeclAttr.cpp

4882 ↗

(On Diff #33762)

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]

include/clang/Basic/Attr.td

894 ↗

(On Diff #33762)

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 ↗

(On Diff #33762)

Done.

896 ↗

(On Diff #33762)

Done.

897 ↗

(On Diff #33762)

Done.

lib/CodeGen/CGCall.cpp

1477 ↗

(On Diff #33762)

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

1480 ↗

(On Diff #33762)

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

lib/Sema/SemaDeclAttr.cpp

4882 ↗

(On Diff #33762)

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]

include/clang/Basic/Attr.td

894 ↗

(On Diff #33882)

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 ↗

(On Diff #33882)

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 ↗

(On Diff #33882)

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 ↗

(On Diff #33882)

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]

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]

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 ↗

(On Diff #33882)

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 ↗

(On Diff #33882)

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 ↗

(On Diff #33882)

OK, done.

lib/Sema/SemaDeclAttr.cpp

4882 ↗

(On Diff #33882)

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]

Made the following changes to address review comments:

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

[aaron.ballman]

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?

[ahatanak]

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]

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.

[aaron.ballman]

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

1683 ↗

(On Diff #39702)

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

1486 ↗

(On Diff #39702)

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

[ahatanak]

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.

[aaron.ballman]

In D12547#287153, @ahatanak wrote:

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]

Address review comments.

[aaron.ballman]

test/SemaCXX/attr-disable-tail-calls.cpp

2 ↗

(On Diff #40011)

Is this file missing an expected-no-diagnostics?

[ahatanak]

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

[aaron.ballman]

LGTM, thank you!

[ahatanak]

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

cfe/trunk/include/clang/Basic/Attr.td

 }
 
 def ReturnsTwice : InheritableAttr {
   let Spellings = [GCC<"returns_twice">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
 }
 
+def DisableTailCalls : InheritableAttr {
+  let Spellings = [GNU<"disable_tail_calls">,
+                   CXX11<"clang", "disable_tail_calls">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Documentation = [DisableTailCallsDocs];
+}
+
 def NoAlias : InheritableAttr {
   let Spellings = [Declspec<"noalias">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [NoAliasDocs];
 }
 
 def NoCommon : InheritableAttr {
   let Spellings = [GCC<"nocommon">];

cfe/trunk/include/clang/Basic/AttrDocs.td

def InternalLinkageDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``internal_linkage`` attribute changes the linkage type of the declaration to internal.
 This is similar to C-style ``static``, but can be used on classes and class methods. When applied to a class definition,
 this attribute affects all methods and static data members of that class.
 This can be used to contain the ABI of a C++ library by excluding unwanted class methods from the export tables.
   }];
 }
+
+def DisableTailCallsDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``disable_tail_calls`` attribute instructs the backend to not perform tail call optimization inside the marked function.
+
+For example:
+
+  .. code-block:: c
+
+    int callee(int);
+
+    int foo(int a) __attribute__((disable_tail_calls)) {
+      return callee(a); // This call is not tail-call optimized.
+    }
+
+Marking virtual functions as ``disable_tail_calls`` is legal.
+
+  .. code-block: c++
+
+    int callee(int);
+
+    class Base {
+    public:
+      [[clang::disable_tail_calls]] virtual int foo1() {
+        return callee(); // This call is not tail-call optimized.
+      }
+    };
+
+    class Derived1 : public Base {
+    public:
+      int foo1() override {
+        return callee(); // This call is tail-call optimized.
+      }
+    };
+
+  }];
+}

cfe/trunk/lib/CodeGen/CGCall.cpp

if (AttrOnCallSite) {
     } else if (CodeGenOpts.OmitLeafFramePointer) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     } else {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     }
 
+    bool DisableTailCalls =
+        CodeGenOpts.DisableTailCalls ||
+        (TargetDecl && TargetDecl->hasAttr<DisableTailCallsAttr>());
     FuncAttrs.addAttribute("disable-tail-calls",
-                           llvm::toStringRef(CodeGenOpts.DisableTailCalls));
+                           llvm::toStringRef(DisableTailCalls));
+
     FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
     FuncAttrs.addAttribute("no-infs-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoInfsFPMath));
     FuncAttrs.addAttribute("no-nans-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoNaNsFPMath));
     FuncAttrs.addAttribute("unsafe-fp-math",
                            llvm::toStringRef(CodeGenOpts.UnsafeFPMath));

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (S.LangOpts.CPlusPlus) {
     return;
   }
 
   if (CommonAttr *CA = S.mergeCommonAttr(D, Attr.getRange(), Attr.getName(),
                                          Attr.getAttributeSpellingListIndex()))
     D->addAttr(CA);
 }
 
+static void handleNakedAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (checkAttrMutualExclusion<DisableTailCallsAttr>(S, D, Attr.getRange(),
+                                                     Attr.getName()))
+    return;
+
+  D->addAttr(::new (S.Context) NakedAttr(Attr.getRange(), S.Context,
+                                         Attr.getAttributeSpellingListIndex()));
+}
+
 static void handleNoReturnAttr(Sema &S, Decl *D, const AttributeList &attr) {
   if (hasDeclarator(D)) return;
 
   if (S.CheckNoReturnAttr(attr)) return;
 
   if (!isa<ObjCMethodDecl>(D)) {
     S.Diag(attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << attr.getName() << ExpectedFunctionOrMethod;
static void handleNotTailCalledAttr(Sema &S, Decl *D,
   if (checkAttrMutualExclusion<AlwaysInlineAttr>(S, D, Attr.getRange(),
                                                  Attr.getName()))
     return;
 
   D->addAttr(::new (S.Context) NotTailCalledAttr(
       Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleDisableTailCallsAttr(Sema &S, Decl *D,
+                                       const AttributeList &Attr) {
+  if (checkAttrMutualExclusion<NakedAttr>(S, D, Attr.getRange(),
+                                          Attr.getName()))
+    return;
+
+  D->addAttr(::new (S.Context) DisableTailCallsAttr(
+      Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+}
+
 static void handleUsedAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     if (VD->hasLocalStorage()) {
       S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
       return;
     }
   } else if (!isFunctionOrMethod(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
case AttributeList::AT_Ownership:
     break;
   case AttributeList::AT_Cold:
     handleColdAttr(S, D, Attr);
     break;
   case AttributeList::AT_Hot:
     handleHotAttr(S, D, Attr);
     break;
   case AttributeList::AT_Naked:
-    handleSimpleAttribute<NakedAttr>(S, D, Attr);
+    handleNakedAttr(S, D, Attr);
     break;
   case AttributeList::AT_NoReturn:
     handleNoReturnAttr(S, D, Attr);
     break;
   case AttributeList::AT_NoThrow:
     handleSimpleAttribute<NoThrowAttr>(S, D, Attr);
     break;
   case AttributeList::AT_CUDAShared:
case AttributeList::AT_Unused:
     handleSimpleAttribute<UnusedAttr>(S, D, Attr);
     break;
   case AttributeList::AT_ReturnsTwice:
     handleSimpleAttribute<ReturnsTwiceAttr>(S, D, Attr);
     break;
   case AttributeList::AT_NotTailCalled:
     handleNotTailCalledAttr(S, D, Attr);
     break;
+  case AttributeList::AT_DisableTailCalls:
+    handleDisableTailCallsAttr(S, D, Attr);
+    break;
   case AttributeList::AT_Used:
     handleUsedAttr(S, D, Attr);
     break;
   case AttributeList::AT_Visibility:
     handleVisibilityAttr(S, D, Attr, false);
     break;
   case AttributeList::AT_TypeVisibility:
     handleVisibilityAttr(S, D, Attr, true);

cfe/trunk/test/CodeGen/attr-disable-tail-calls.c

-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -mdisable-tail-calls -o - | FileCheck %s -check-prefix=CHECK -check-prefix=DISABLE
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | FileCheck %s -check-prefix=CHECK -check-prefix=ENABLE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -mdisable-tail-calls -o - | FileCheck %s -check-prefix=DISABLE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.7.0 %s -emit-llvm -o - | FileCheck %s -check-prefix=ENABLE
 
-// CHECK: define i32 @f1() [[ATTR:#[0-9]+]] {
+// DISABLE: define i32 @f1() [[ATTRTRUE:#[0-9]+]] {
+// DISABLE: define i32 @f2() [[ATTRTRUE]] {
+// ENABLE: define i32 @f1() [[ATTRFALSE:#[0-9]+]] {
+// ENABLE: define i32 @f2() [[ATTRTRUE:#[0-9]+]] {
 
 int f1() {
   return 0;
 }
 
-// DISABLE: attributes [[ATTR]] = { {{.*}} "disable-tail-calls"="true" {{.*}} }
-// ENABLE: attributes [[ATTR]] = { {{.*}} "disable-tail-calls"="false" {{.*}} }
+int f2() __attribute__((disable_tail_calls)) {
+  return 0;
+}
+
+// DISABLE: attributes [[ATTRTRUE]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }
+// ENABLE: attributes [[ATTRFALSE]] = { {{.*}}"disable-tail-calls"="false"{{.*}} }
+// ENABLE: attributes [[ATTRTRUE]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }

cfe/trunk/test/CodeGenCXX/attr-disable-tail-calls.cpp

+// RUN: %clang_cc1 -triple=x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+class B {
+public:
+  [[clang::disable_tail_calls]] virtual int m1() { return 1; }
+  virtual int m2() { return 2; }
+  int m3() { return 3; }
+  [[clang::disable_tail_calls]] int m4();
+};
+
+class D : public B {
+public:
+  int m1() override { return 11; }
+  [[clang::disable_tail_calls]] int m2() override { return 22; }
+};
+
+int foo1() {
+  B *b = new B;
+  D *d = new D;
+  int t = 0;
+  t += b->m1() + b->m2() + b->m3() + b->m4();
+  t += d->m1() + d->m2();
+  return t;
+}
+
+// CHECK: define linkonce_odr i32 @_ZN1B2m3Ev(%class.B* %this) [[ATTRFALSE:#[0-9]+]]
+// CHECK: declare i32 @_ZN1B2m4Ev(%class.B*) [[ATTRTRUE0:#[0-9]+]]
+// CHECK: define linkonce_odr i32 @_ZN1B2m1Ev(%class.B* %this) unnamed_addr [[ATTRTRUE1:#[0-9]+]]
+// CHECK: define linkonce_odr i32 @_ZN1B2m2Ev(%class.B* %this) unnamed_addr [[ATTRFALSE:#[0-9]+]]
+// CHECK: define linkonce_odr i32 @_ZN1D2m1Ev(%class.D* %this) unnamed_addr [[ATTRFALSE:#[0-9]+]]
+// CHECK: define linkonce_odr i32 @_ZN1D2m2Ev(%class.D* %this) unnamed_addr [[ATTRTRUE1:#[0-9]+]]
+
+// CHECK: attributes [[ATTRFALSE]] = { {{.*}}"disable-tail-calls"="false"{{.*}} }
+// CHECK: attributes [[ATTRTRUE0]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }
+// CHECK: attributes [[ATTRTRUE1]] = { {{.*}}"disable-tail-calls"="true"{{.*}} }

cfe/trunk/test/Sema/attr-disable-tail-calls.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void __attribute__((disable_tail_calls,naked)) foo1(int a) { // expected-error {{'disable_tail_calls' and 'naked' attributes are not compatible}} expected-note {{conflicting attribute is here}}
+  __asm__("");
+}
+
+void __attribute__((naked,disable_tail_calls)) foo2(int a) { // expected-error {{'naked' and 'disable_tail_calls' attributes are not compatible}} expected-note {{conflicting attribute is here}}
+  __asm__("");
+}
+
+int g0 __attribute__((disable_tail_calls)); // expected-warning {{'disable_tail_calls' attribute only applies to functions and methods}}
+
+int foo3(int a) __attribute__((disable_tail_calls("abc"))); // expected-error {{'disable_tail_calls' attribute takes no arguments}}

cfe/trunk/test/SemaCXX/attr-disable-tail-calls.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+class B {
+public:
+  [[clang::disable_tail_calls]] virtual int foo1() { return 1; }
+  [[clang::disable_tail_calls]] int foo2() { return 2; }
+};