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

[Aaron Ballman via cfe-commits]

On Wed, Sep 16, 2015 at 12:47 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:
> 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:
>> 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.
> Yes, that's correct. The new function attribute we are adding shouldn't disable other optimizations that are normally run at -O2 or -O3.

Okay -- with this and your other explanations, I now understand why
this attribute is orthogonal to optnone. Thanks!

> ================
> Comment at: include/clang/Basic/Attr.td:896
> @@ +895,3 @@
> +  let Spellings = [GNU<"disable_tail_calls">,
> +                   CXX11<"clang", "disable_tail_calls">];
> +  let Subjects = SubjectList<[Function, ObjCMethod]>;
> ----------------
> aaron.ballman wrote:
>> Should this attribute be plural? Are there multiple tail calls within a single method that can be optimized away?
> 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".

I'm holding off on having an opinion on the name until I have an idea
as to the other planned attribute's name, but I basically think the
plural is reasonable if the purpose is to disable TCO on the functions
called within the marked function. But we may want to consider making
the name more obvious. The majority of the attributes we support
appertain to the behavior of the declaration itself, not to entities
*within* the declaration. However, it seems like no_sanitize,
no_sanitize_thread, et al are similar concepts. Perhaps we want this
to be named "no_tail_calls" instead? Just something to think about.

> ================
> Comment at: include/clang/Basic/AttrDocs.td:1619
> @@ +1618,3 @@
> +  let Content = [{
> +Use ``__attribute__((disable_tail_calls)))`` to instruct the backend not to do tail call optimization.
> +  }];
> ----------------
> aaron.ballman wrote:
>> 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.
> OK, done.
>
> ================
> Comment at: lib/Sema/SemaDeclAttr.cpp:4882
> @@ +4881,3 @@
> +  case AttributeList::AT_DisableTailCalls:
> +    handleSimpleAttribute<DisableTailCallsAttr>(S, D, Attr);
> +    break;
> ----------------
> aaron.ballman wrote:
>> 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.
> 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.

__declspec(naked) behaves differently than __attribute__((naked)), but
I'm not certain whether that changes the reasoning
(https://msdn.microsoft.com/en-us/library/h5w10wxs.aspx).

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

That makes sense to me.

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

If a user finds something, we can handle it at that time. Thank you
for looking into this!

> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -1612,3 +1612,10 @@
>  arguments, with arbitrary offsets.
>    }];
>  }
> +
> +def DisableTailCallsDocs : Documentation {
> +  let Category = DocCatFunction;
> +  let Content = [{
> +The ``disable_tail_calls`` attribute instructs the backend to not perform tail call optimization.

The documentation should be updated to clarify the semantics of the
attribute. Right now it's ambiguous as to which TCOs are disabled.

> +  }];
> +}
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -1696,6 +1696,18 @@
>                                     Attr.getAttributeSpellingListIndex()));
>  }
>
> +static void handleDisableTailCallsAttr(Sema &S, Decl *D,
> +                                       const AttributeList &Attr) {
> +  if (auto *A = D->getAttr<NakedAttr>()) {
> +    S.Diag(Attr.getLoc(), diag::err_attributes_are_not_compatible)
> +        << Attr.getName() << A;
> +    return;
> +  }

Please use checkAttrMutualExclusion() instead.

~Aaron