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

[Akira Hatanaka via cfe-commits]

On Wed, Sep 16, 2015 at 8:05 AM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> 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.
>
>
I think something similar to no_sanitize makes sense.

The tentative names I had were notailcall for this attribute and notail for
the other, but I'm open to suggestions if there are better names.

I'll send a patch to support the other attribute today.

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

gcc seems to take a stricter stance on what is allowed inside naked
functions, but I think it's still safe to say it's a mistake to attach
disable-tail-calls to a naked function (because you have no control over
it), regardless of whether it's an msvc or gcc attribute.

https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html#ARM-Function-Attributes


> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150916/db314ba7/attachment.html>