Re: [PATCH] Add cold attribute to one time construction APIs

[Jan Hubicka <hubicka@ucw.cz>]

> On Tue, Aug 18, 2020 at 4:36 PM Jonathan Wakely via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On 17/08/20 18:15 +0000, Aditya K via Libstdc++ wrote:
> > >This would help compiler optimize local static objects.
> > >
> > >Added changelog.
> >
> > Please don't :-)
> >
> > GCC patch policies always said NOT to change the ChangeLog in the
> > patch, because the ChangeLog files change so rapidly that it means by
> > the time you've sent the patch, it no longer applies.
> >
> > Current GCC policies are that NOBODY changes the ChangeLog files, they
> > are autogenerated from Git commit logs once per day.
> >
> > So please just include the proposed ChangeLog entry as the Git commit
> > message in the body of your email.
> >
> > Patch for libstdc++ need to go to both the libstdc++ list and the
> > gcc-patches list, in the same email. Not sent once to each list as
> > separate emails.
> >
> > >
> > >```
> > >From c6cba40e0434147db89d3af05b598782cde651e3 Mon Sep 17 00:00:00 2001
> > >From: Aditya Kumar <1894981+hiraditya@users.noreply.github.com>
> > >Date: Thu, 13 Aug 2020 09:41:34 -0700
> > >Subject: [PATCH] Add cold attribute to one time construction APIs
> > >
> > >__cxa_guard_acquire is used for only one purpose,
> > >namely guarding local static variable initialization,
> > >and since that purpose is definitionally cold, it should be attributed as cold.
> >
> > Definitionally isn't a word. Attributed is a word, but it doesn't mean
> > marked with an attribute.
> >
> > >Similarly for __cxa_guard_release and __cxa_guard_abort
> > >---
> > > libstdc++-v3/ChangeLog              | 5 +++++
> > > libstdc++-v3/include/bits/c++config | 5 +++++
> > > libstdc++-v3/libsupc++/cxxabi.h     | 6 +++---
> > > 3 files changed, 13 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
> > >index fe6884bf3..86b707ac7 100644
> > >--- a/libstdc++-v3/ChangeLog
> > >+++ b/libstdc++-v3/ChangeLog
> > >@@ -1,3 +1,8 @@
> > >+2020-08-17  Aditya Kumar  <hiraditya@msn.com>
> > >+      * libstdc++-v3/include/bits/c++config: Add _GLIBCXX_NOTHROW attribute
> > >+      * libstdc++-v3/libsupc++/cxxabi.h (__cxa_guard_acquire, __cxa_guard_release,
> > >+      __cxa_guard_abort): Add _GLIBCXX_NOTHROW attribute.
> >
> > The changelog format is wrong. There should be a blank line after the
> > date+author line, and you're adding _GLIBCXX_COLD not
> > _GLIBCXX_NOTHROW. But it shouldn't be here at all as explained above.
> >
> > > 2020-08-14  Lewis Hyatt  <lhyatt@gmail.com>
> > >
> > >       * testsuite/lib/libstdc++.exp: Use the new option
> > >diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
> > >index b1fad59d4..f6f954eef 100644
> > >--- a/libstdc++-v3/include/bits/c++config
> > >+++ b/libstdc++-v3/include/bits/c++config
> > >@@ -42,6 +42,7 @@
> > > //   _GLIBCXX_NORETURN
> > > //   _GLIBCXX_NOTHROW
> > > //   _GLIBCXX_VISIBILITY
> > >+//   _GLIBCXX_COLD
> > > #ifndef _GLIBCXX_PURE
> > > # define _GLIBCXX_PURE __attribute__ ((__pure__))
> > > #endif
> > >@@ -74,6 +75,10 @@
> > > # define _GLIBCXX_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
> > > #endif
> > >
> > >+#ifndef _GLIBCXX_COLD
> > >+# define _GLIBCXX_COLD __attribute__ ((cold))
> > >+#endif
> >
> > "cold" is not a reserved name so this needs to be __cold__.
> >
> > I'm not sure we really need it in <bits/c++config.h> if we only use it
> > in one file, but maybe we'll find more uses for it later.
> >
> > >diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h
> > >index 000713ecd..24c1366e2 100644
> > >--- a/libstdc++-v3/libsupc++/cxxabi.h
> > >+++ b/libstdc++-v3/libsupc++/cxxabi.h
> > >@@ -115,13 +115,13 @@ namespace __cxxabiv1
> > >                   void (*__dealloc) (void*, size_t));
> > >
> > >   int
> > >-  __cxa_guard_acquire(__guard*);
> > >+  __cxa_guard_acquire(__guard*) _GLIBCXX_COLD;
> >
> > The GCC manual says that functions marked cold will be optimized for
> > size not for speed. Is that really what we want here?
> >
> > It makes sense to put them in a cold section, but I think we still
> > want them to be optimized for speed, don't we?
> >
> > I've attached a patch addressing the issues above, but I'd like to
> > know whether the change to how the functions are optimized is
> > desirable, or even noticable.
> 
> Hmm, but the functions are always executed?  The .cold sections
> are so they are not paged in and this particular case would defeat
> this purpose?

We have internal attribute for functions executed at startup and exit.
We could export that via attribute to users if that helps.  Note that
GCC is able to propagate this info in easy enough examples.

But if I am right, this is used for EH and we generally drop EH into
cold section.

Honza
> 
> Richard.
> 
> >