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

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]

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]

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]

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

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]

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

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().

[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.

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">];

include/clang/Basic/AttrDocs.td

 def NoAliasDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``noalias`` attribute indicates that the only memory accesses inside
 function are loads and stores from objects pointed to by its pointer-typed
 arguments, with arbitrary offsets.
   }];
 }
+
+def DisableTailCallsDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use ``__attribute__((disable_tail_calls)))`` to instruct the backend not to do tail call optimization.
+  }];
+}

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 =
+        (TargetDecl && TargetDecl->hasAttr<DisableTailCallsAttr>()) ||
+        CodeGenOpts.DisableTailCalls;
     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));
     FuncAttrs.addAttribute("use-soft-float",
                            llvm::toStringRef(CodeGenOpts.SoftFloat));

lib/Sema/SemaDeclAttr.cpp

case AttributeList::AT_ObjCRequiresPropertyDefs:
     handleSimpleAttribute<ObjCRequiresPropertyDefsAttr>(S, D, Attr);
     break;
   case AttributeList::AT_Unused:
     handleSimpleAttribute<UnusedAttr>(S, D, Attr);
     break;
   case AttributeList::AT_ReturnsTwice:
     handleSimpleAttribute<ReturnsTwiceAttr>(S, D, Attr);
     break;
+  case AttributeList::AT_DisableTailCalls:
+    handleSimpleAttribute<DisableTailCallsAttr>(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);

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" {{.*}} }