[PATCH] D12547: Add support for function attribute "disable_tail_calls"

[Akira Hatanaka via cfe-commits]

ahatanak added inline comments.

================
Comment at: include/clang/Basic/Attr.td:894
@@ -893,1 +893,3 @@
 
+def DisableTailCalls : InheritableAttr {
+  let Spellings = [GNU<"disable_tail_calls">];
----------------
aaron.ballman wrote:
> 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?
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.

================
Comment at: include/clang/Basic/Attr.td:895
@@ +894,3 @@
+def DisableTailCalls : InheritableAttr {
+  let Spellings = [GNU<"disable_tail_calls">];
+  let Subjects = SubjectList<[Function]>;
----------------
aaron.ballman wrote:
> Do we also want a C++ spelling for this attribute, under the clang namespace?
Done.

================
Comment at: include/clang/Basic/Attr.td:896
@@ +895,3 @@
+  let Spellings = [GNU<"disable_tail_calls">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> Should this also apply to Objective-C methods?
Done.

================
Comment at: include/clang/Basic/Attr.td:897
@@ +896,3 @@
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> Please document this attribute.
Done.

================
Comment at: lib/CodeGen/CGCall.cpp:1477
@@ -1476,3 +1476,3 @@
 
-    FuncAttrs.addAttribute("disable-tail-calls",
-                           llvm::toStringRef(CodeGenOpts.DisableTailCalls));
+    if ((TargetDecl && TargetDecl->hasAttr<DisableTailCallsAttr>()) ||
+        CodeGenOpts.DisableTailCalls)
----------------
aaron.ballman wrote:
> Better to move this into the block that already tests TargetDecl (around line 1403 or so).
It looks like moving this code into the block would make the code mode complex, so I followed your suggestion below.

================
Comment at: lib/CodeGen/CGCall.cpp:1480
@@ +1479,3 @@
+      FuncAttrs.addAttribute("disable-tail-calls", "true");
+    else
+      FuncAttrs.addAttribute("disable-tail-calls", "false");
----------------
aaron.ballman wrote:
> Can do this in a single line with ?:
I defined a boolean variable and passed it to llvm::toStringRef to avoid calling addAttribute in two places.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4882
@@ +4881,3 @@
+  case AttributeList::AT_DisableTailCalls:
+    handleSimpleAttribute<DisableTailCallsAttr>(S, D, Attr);
+    break;
----------------
aaron.ballman wrote:
> Are there semantic checks we would like to perform on the declaration for nonsense uses of this attribute, such as functions marked [[noreturn]], etc?
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


http://reviews.llvm.org/D12547