* Re: Add cold attribute to one time construction APIs [not found] ` <BYAPR08MB423270DD470245D666E7FC04B6430@BYAPR08MB4232.namprd08.prod.outlook.com> @ 2020-08-13 17:13 ` Jonathan Wakely 2020-08-13 18:58 ` Aditya K 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2020-08-13 17:13 UTC (permalink / raw) To: Aditya K; +Cc: libstdc++, gcc-patches Please CC the libstdc++ list on all libstdc++ patches. On Thu, 13 Aug 2020 at 17:51, Aditya K <hiraditya@msn.com> wrote: > > Revised patch with _GLIBCXX_COLD added at the end. > > ``` > commit 3dc9f9a8461b1c88e991ceb517e5fdd81f268d1e > Author: Aditya Kumar <1894981+hiraditya@users.noreply.github.com> > Date: Thu Aug 13 09:41:34 2020 -0700 > > 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. > Similarly for __cxa_guard_release and __cxa_guard_abort > > 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 > @@ -35,20 +35,21 @@ > > // The datestamp of the C++ library in compressed ISO date format. > #define __GLIBCXX__ > > // Macros for various attributes. > // _GLIBCXX_PURE > // _GLIBCXX_CONST > // _GLIBCXX_NORETURN > // _GLIBCXX_NOTHROW > // _GLIBCXX_VISIBILITY > +// _GLIBCXX_COLD > #ifndef _GLIBCXX_PURE > # define _GLIBCXX_PURE __attribute__ ((__pure__)) > #endif > > #ifndef _GLIBCXX_CONST > # define _GLIBCXX_CONST __attribute__ ((__const__)) > #endif > > #ifndef _GLIBCXX_NORETURN > # define _GLIBCXX_NORETURN __attribute__ ((__noreturn__)) > @@ -67,20 +68,24 @@ > #define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY > > #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY > # define _GLIBCXX_VISIBILITY(V) __attribute__ ((__visibility__ (#V))) > #else > // If this is not supplied by the OS-specific or CPU-specific > // headers included below, it will be defined to an empty default. > # define _GLIBCXX_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY(V) > #endif > > +#ifndef _GLIBCXX_COLD > +# define _GLIBCXX_COLD __attribute__ ((cold)) > +#endif > + > // Macros for deprecated attributes. > // _GLIBCXX_USE_DEPRECATED > // _GLIBCXX_DEPRECATED > // _GLIBCXX17_DEPRECATED > // _GLIBCXX20_DEPRECATED( string-literal ) > #ifndef _GLIBCXX_USE_DEPRECATED > # define _GLIBCXX_USE_DEPRECATED 1 > #endif > > #if defined(__DEPRECATED) && (__cplusplus >= 201103L) > 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 > @@ -108,27 +108,27 @@ namespace __cxxabiv1 > __cxa_vec_delete2(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*)); > > void > __cxa_vec_delete3(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*, size_t)); > > int > - __cxa_guard_acquire(__guard*); > + __cxa_guard_acquire(__guard*) _GLIBCXX_COLD; > > void > - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > void > - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > // DSO destruction. > int > __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW; > > void > __cxa_finalize(void*); > > // TLS destruction. > int > ``` > > From: Aditya K > Sent: Thursday, August 13, 2020 10:47 AM > To: Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org>; jwakely.gcc@gmail.com <jwakely.gcc@gmail.com> > Subject: Add cold attribute to one time construction APIs > > This would help compiler optimize local static objects. > > ``` > commit e2f299679ddf56a6d6d71ea9d589cd76b2ca107b > Author: Aditya Kumar <1894981+hiraditya@users.noreply.github.com> > Date: Thu Aug 13 09:41:34 2020 -0700 > > 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. > Similarly for __cxa_guard_release and __cxa_guard_abort > > diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config > index b1fad59d4..359e955a7 100644 > --- a/libstdc++-v3/include/bits/c++config > +++ b/libstdc++-v3/include/bits/c++config > @@ -39,20 +39,24 @@ > // Macros for various attributes. > // _GLIBCXX_PURE > // _GLIBCXX_CONST > // _GLIBCXX_NORETURN > // _GLIBCXX_NOTHROW > // _GLIBCXX_VISIBILITY > #ifndef _GLIBCXX_PURE > # define _GLIBCXX_PURE __attribute__ ((__pure__)) > #endif > > +#ifndef _GLIBCXX_COLD > +# define _GLIBCXX_COLD __attribute__ ((cold)) > +#endif > + > #ifndef _GLIBCXX_CONST > # define _GLIBCXX_CONST __attribute__ ((__const__)) > #endif > > #ifndef _GLIBCXX_NORETURN > # define _GLIBCXX_NORETURN __attribute__ ((__noreturn__)) > #endif > > // See below for C++ > #ifndef _GLIBCXX_NOTHROW > 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 > @@ -108,27 +108,27 @@ namespace __cxxabiv1 > __cxa_vec_delete2(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*)); > > void > __cxa_vec_delete3(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*, size_t)); > > int > - __cxa_guard_acquire(__guard*); > + __cxa_guard_acquire(__guard*) _GLIBCXX_COLD; > > void > - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > void > - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > // DSO destruction. > int > __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW; > > void > __cxa_finalize(void*); > > // TLS destruction. > int > ``` ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Add cold attribute to one time construction APIs 2020-08-13 17:13 ` Add cold attribute to one time construction APIs Jonathan Wakely @ 2020-08-13 18:58 ` Aditya K 0 siblings, 0 replies; 9+ messages in thread From: Aditya K @ 2020-08-13 18:58 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches sure. -- From: Jonathan Wakely <jwakely.gcc@gmail.com> Sent: Thursday, August 13, 2020 11:13 AM To: Aditya K <hiraditya@msn.com> Cc: libstdc++ <libstdc++@gcc.gnu.org>; gcc-patches <gcc-patches@gcc.gnu.org> Subject: Re: Add cold attribute to one time construction APIs Please CC the libstdc++ list on all libstdc++ patches. On Thu, 13 Aug 2020 at 17:51, Aditya K <hiraditya@msn.com> wrote: > > Revised patch with _GLIBCXX_COLD added at the end. > > ``` > commit 3dc9f9a8461b1c88e991ceb517e5fdd81f268d1e > Author: Aditya Kumar <1894981+hiraditya@users.noreply.github.com> > Date: Thu Aug 13 09:41:34 2020 -0700 > > 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. > Similarly for __cxa_guard_release and __cxa_guard_abort > > 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 > @@ -35,20 +35,21 @@ > > // The datestamp of the C++ library in compressed ISO date format. > #define __GLIBCXX__ > > // Macros for various attributes. > // _GLIBCXX_PURE > // _GLIBCXX_CONST > // _GLIBCXX_NORETURN > // _GLIBCXX_NOTHROW > // _GLIBCXX_VISIBILITY > +// _GLIBCXX_COLD > #ifndef _GLIBCXX_PURE > # define _GLIBCXX_PURE __attribute__ ((__pure__)) > #endif > > #ifndef _GLIBCXX_CONST > # define _GLIBCXX_CONST __attribute__ ((__const__)) > #endif > > #ifndef _GLIBCXX_NORETURN > # define _GLIBCXX_NORETURN __attribute__ ((__noreturn__)) > @@ -67,20 +68,24 @@ > #define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY > > #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY > # define _GLIBCXX_VISIBILITY(V) __attribute__ ((__visibility__ (#V))) > #else > // If this is not supplied by the OS-specific or CPU-specific > // headers included below, it will be defined to an empty default. > # define _GLIBCXX_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY(V) > #endif > > +#ifndef _GLIBCXX_COLD > +# define _GLIBCXX_COLD __attribute__ ((cold)) > +#endif > + > // Macros for deprecated attributes. > // _GLIBCXX_USE_DEPRECATED > // _GLIBCXX_DEPRECATED > // _GLIBCXX17_DEPRECATED > // _GLIBCXX20_DEPRECATED( string-literal ) > #ifndef _GLIBCXX_USE_DEPRECATED > # define _GLIBCXX_USE_DEPRECATED 1 > #endif > > #if defined(__DEPRECATED) && (__cplusplus >= 201103L) > 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 > @@ -108,27 +108,27 @@ namespace __cxxabiv1 > __cxa_vec_delete2(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*)); > > void > __cxa_vec_delete3(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*, size_t)); > > int > - __cxa_guard_acquire(__guard*); > + __cxa_guard_acquire(__guard*) _GLIBCXX_COLD; > > void > - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > void > - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > // DSO destruction. > int > __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW; > > void > __cxa_finalize(void*); > > // TLS destruction. > int > ``` > > From: Aditya K > Sent: Thursday, August 13, 2020 10:47 AM > To: Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org>; jwakely.gcc@gmail.com <jwakely.gcc@gmail.com> > Subject: Add cold attribute to one time construction APIs > > This would help compiler optimize local static objects. > > ``` > commit e2f299679ddf56a6d6d71ea9d589cd76b2ca107b > Author: Aditya Kumar <1894981+hiraditya@users.noreply.github.com> > Date: Thu Aug 13 09:41:34 2020 -0700 > > 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. > Similarly for __cxa_guard_release and __cxa_guard_abort > > diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config > index b1fad59d4..359e955a7 100644 > --- a/libstdc++-v3/include/bits/c++config > +++ b/libstdc++-v3/include/bits/c++config > @@ -39,20 +39,24 @@ > // Macros for various attributes. > // _GLIBCXX_PURE > // _GLIBCXX_CONST > // _GLIBCXX_NORETURN > // _GLIBCXX_NOTHROW > // _GLIBCXX_VISIBILITY > #ifndef _GLIBCXX_PURE > # define _GLIBCXX_PURE __attribute__ ((__pure__)) > #endif > > +#ifndef _GLIBCXX_COLD > +# define _GLIBCXX_COLD __attribute__ ((cold)) > +#endif > + > #ifndef _GLIBCXX_CONST > # define _GLIBCXX_CONST __attribute__ ((__const__)) > #endif > > #ifndef _GLIBCXX_NORETURN > # define _GLIBCXX_NORETURN __attribute__ ((__noreturn__)) > #endif > > // See below for C++ > #ifndef _GLIBCXX_NOTHROW > 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 > @@ -108,27 +108,27 @@ namespace __cxxabiv1 > __cxa_vec_delete2(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*)); > > void > __cxa_vec_delete3(void* __array_address, size_t __element_size, > size_t __padding_size, __cxa_cdtor_type __destructor, > void (*__dealloc) (void*, size_t)); > > int > - __cxa_guard_acquire(__guard*); > + __cxa_guard_acquire(__guard*) _GLIBCXX_COLD; > > void > - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > void > - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; > + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; > > // DSO destruction. > int > __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW; > > void > __cxa_finalize(void*); > > // TLS destruction. > int > ``` ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Add cold attribute to one time construction APIs [not found] <BYAPR08MB423235937D163AE584BD8E3CB6430@BYAPR08MB4232.namprd08.prod.outlook.com> [not found] ` <BYAPR08MB423270DD470245D666E7FC04B6430@BYAPR08MB4232.namprd08.prod.outlook.com> @ 2020-08-17 18:15 ` Aditya K 2020-08-18 14:35 ` Jonathan Wakely 1 sibling, 1 reply; 9+ messages in thread From: Aditya K @ 2020-08-17 18:15 UTC (permalink / raw) To: libstdc++, jwakely.gcc This would help compiler optimize local static objects. Added changelog. ``` 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. 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. + 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 + // Macros for deprecated attributes. // _GLIBCXX_USE_DEPRECATED // _GLIBCXX_DEPRECATED 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; void - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; void - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; // DSO destruction. int -- 2.13.5 ``` ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-17 18:15 ` [PATCH] " Aditya K @ 2020-08-18 14:35 ` Jonathan Wakely 2020-08-18 14:38 ` Jonathan Wakely 2020-08-24 8:06 ` Richard Biener 0 siblings, 2 replies; 9+ messages in thread From: Jonathan Wakely @ 2020-08-18 14:35 UTC (permalink / raw) To: Aditya K; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3802 bytes --] 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. [-- Attachment #2: cold.patch.txt --] [-- Type: text/plain, Size: 2062 bytes --] commit fadd79179f93c82c2935fdfe17a2ab1586b4e70f Author: Aditya Kumar <hiraditya@msn.com> Date: Tue Aug 18 15:22:24 2020 libstdc++: Add cold attribute to one time construction APIs __cxa_guard_acquire is used for only one purpose, namely guarding local static variable initialization. Since that purpose happens rarely, it should be marked with the 'cold' attribute. Similarly for __cxa_guard_release and __cxa_guard_abort. libstdc++-v3/ChangeLog: 2020-08-18 Aditya Kumar <hiraditya@msn.com> * include/bits/c++config (_GLIBCXX_COLD): Define. * libsupc++/cxxabi.h (__cxa_guard_acquire, __cxa_guard_release) (__cxa_guard_abort): Add _GLIBCXX_COLD. diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config index b1fad59d4b3..a135de00a50 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 + // Macros for deprecated attributes. // _GLIBCXX_USE_DEPRECATED // _GLIBCXX_DEPRECATED diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h index 000713ecdf8..24c1366e271 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; void - __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW; + __cxa_guard_release(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; void - __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW; + __cxa_guard_abort(__guard*) _GLIBCXX_NOTHROW _GLIBCXX_COLD; // DSO destruction. int ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-18 14:35 ` Jonathan Wakely @ 2020-08-18 14:38 ` Jonathan Wakely 2020-08-24 8:06 ` Richard Biener 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Wakely @ 2020-08-18 14:38 UTC (permalink / raw) To: Aditya K; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3157 bytes --] On 18/08/20 15:35 +0100, Jonathan Wakely 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've just pushed the attached patch which ensures we don't use the non-reserved form of the attribute. Tested x86_64-linux, committed to trunk. [-- Attachment #2: patch.txt --] [-- Type: text/x-patch, Size: 4210 bytes --] commit 6c1a58b7fbdaa8ac00957fccfb379af163309311 Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Aug 18 15:37:14 2020 libstdc++: Add "cold" to tests for reserved attribute names libstdc++-v3/ChangeLog: * testsuite/17_intro/headers/c++1998/all_attributes.cc: Check "cold" isn't used in the library. Also check <cxxabi.h>. * testsuite/17_intro/headers/c++2011/all_attributes.cc: Likewise. * testsuite/17_intro/headers/c++2014/all_attributes.cc: Likewise. * testsuite/17_intro/headers/c++2017/all_attributes.cc: Likewise. * testsuite/17_intro/headers/c++2020/all_attributes.cc: Likewise. diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc b/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc index 73a20e346e4..dd28429e1d5 100644 --- a/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc +++ b/libstdc++-v3/testsuite/17_intro/headers/c++1998/all_attributes.cc @@ -21,6 +21,7 @@ // Ensure the library only uses the __name__ form for attributes. // Don't test 'const' because it is reserved anyway. #define abi_tag 1 +#define cold 1 #ifndef __APPLE__ // darwin headers use these, see PR 64883 # define always_inline 1 @@ -36,6 +37,7 @@ #endif #include <bits/extc++.h> +#include <cxxabi.h> int main() diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc b/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc index c0a79e06ddc..db00a33f6a3 100644 --- a/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc +++ b/libstdc++-v3/testsuite/17_intro/headers/c++2011/all_attributes.cc @@ -21,6 +21,7 @@ // Ensure the library only uses the __name__ form for attributes. // Don't test 'const' and 'noreturn' because they are reserved anyway. #define abi_tag 1 +#define cold 1 #ifndef __APPLE__ // darwin headers use these, see PR 64883 # define always_inline 1 @@ -35,6 +36,7 @@ #endif #include <bits/extc++.h> +#include <cxxabi.h> int main() diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc b/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc index 1e3ba0888a0..3322c2c1362 100644 --- a/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc +++ b/libstdc++-v3/testsuite/17_intro/headers/c++2014/all_attributes.cc @@ -21,6 +21,7 @@ // Ensure the library only uses the __name__ form for attributes. // Don't test 'const' and 'noreturn' because they are reserved anyway. #define abi_tag 1 +#define cold 1 #ifndef __APPLE__ // darwin headers use these, see PR 64883 # define always_inline 1 @@ -35,6 +36,7 @@ #endif #include <bits/extc++.h> +#include <cxxabi.h> int main() diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc b/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc index 035526e24cb..a0d67739d71 100644 --- a/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc +++ b/libstdc++-v3/testsuite/17_intro/headers/c++2017/all_attributes.cc @@ -21,6 +21,7 @@ // Ensure the library only uses the __name__ form for attributes. // Don't test 'const' and 'noreturn' because they are reserved anyway. #define abi_tag 1 +#define cold 1 #ifndef __APPLE__ // darwin headers use these, see PR 64883 # define always_inline 1 @@ -34,6 +35,7 @@ #endif #include <bits/extc++.h> +#include <cxxabi.h> int main() diff --git a/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc b/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc index 6f5ea460fac..da8f0e415a3 100644 --- a/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc +++ b/libstdc++-v3/testsuite/17_intro/headers/c++2020/all_attributes.cc @@ -21,6 +21,7 @@ // Ensure the library only uses the __name__ form for attributes. // Don't test 'const' and 'noreturn' because they are reserved anyway. #define abi_tag 1 +#define cold 1 #ifndef __APPLE__ // darwin headers use these, see PR 64883 # define always_inline 1 @@ -34,6 +35,7 @@ #endif #include <bits/extc++.h> +#include <cxxabi.h> int main() ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-18 14:35 ` Jonathan Wakely 2020-08-18 14:38 ` Jonathan Wakely @ 2020-08-24 8:06 ` Richard Biener 2020-08-24 8:09 ` Jan Hubicka 1 sibling, 1 reply; 9+ messages in thread From: Richard Biener @ 2020-08-24 8:06 UTC (permalink / raw) To: Jonathan Wakely, Jan Hubicka; +Cc: Aditya K, libstdc++, GCC Patches 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? Richard. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-24 8:06 ` Richard Biener @ 2020-08-24 8:09 ` Jan Hubicka 2020-08-24 8:45 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jan Hubicka @ 2020-08-24 8:09 UTC (permalink / raw) To: Richard Biener; +Cc: Jonathan Wakely, Aditya K, libstdc++, GCC Patches > 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. > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-24 8:09 ` Jan Hubicka @ 2020-08-24 8:45 ` Jonathan Wakely 2020-08-24 8:55 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2020-08-24 8:45 UTC (permalink / raw) To: Jan Hubicka; +Cc: Richard Biener, libstdc++, GCC Patches, Aditya K On 24/08/20 10:09 +0200, Jan Hubicka wrote: >> 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 They're not *guaranteed* to be executed. But they're executed every time control passes over the definition of a local static variable. So making them cold is probably wrong. The __cxa_guard_abort function could be cold, because that's only used if the local static being constructed throws an exception from its constructor. But guard_acquire and guard_release could theoretically be called frequently in hot code (though it's not a good idea to make a hot function use local statics with non-trivial initialization, precisely because that needs to call these external functions). >> 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. These are for local statics, which are not constructed at startup. They are destroyed at exit though. >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. These are for constructing local static variables, not for EH. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Add cold attribute to one time construction APIs 2020-08-24 8:45 ` Jonathan Wakely @ 2020-08-24 8:55 ` Jonathan Wakely 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Wakely @ 2020-08-24 8:55 UTC (permalink / raw) To: Jan Hubicka, Richard Biener, libstdc++, GCC Patches, Aditya K On 24/08/20 09:45 +0100, Jonathan Wakely via Libstdc++ wrote: >On 24/08/20 10:09 +0200, Jan Hubicka wrote: >>>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 > >They're not *guaranteed* to be executed. But they're executed every >time control passes over the definition of a local static variable. > >So making them cold is probably wrong. > >The __cxa_guard_abort function could be cold, because that's only used >if the local static being constructed throws an exception from its >constructor. But guard_acquire and guard_release could theoretically >be called frequently in hot code (though it's not a good idea to make >a hot function use local statics with non-trivial initialization, >precisely because that needs to call these external functions). > >>>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. > >These are for local statics, which are not constructed at startup. >They are destroyed at exit though. But these functions aren't used for the destruction, atexit is used to register the destruction. >>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. > >These are for constructing local static variables, not for EH. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-24 8:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <BYAPR08MB423235937D163AE584BD8E3CB6430@BYAPR08MB4232.namprd08.prod.outlook.com>
[not found] ` <BYAPR08MB423270DD470245D666E7FC04B6430@BYAPR08MB4232.namprd08.prod.outlook.com>
2020-08-13 17:13 ` Add cold attribute to one time construction APIs Jonathan Wakely
2020-08-13 18:58 ` Aditya K
2020-08-17 18:15 ` [PATCH] " Aditya K
2020-08-18 14:35 ` Jonathan Wakely
2020-08-18 14:38 ` Jonathan Wakely
2020-08-24 8:06 ` Richard Biener
2020-08-24 8:09 ` Jan Hubicka
2020-08-24 8:45 ` Jonathan Wakely
2020-08-24 8:55 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).