| Message ID | 20240107203259.1705373-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: reduce std::variant template instantiation depth | expand |
On Sun, 7 Jan 2024, Patrick Palka wrote: > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > -- >8 -- > > The recursively defined constraints on _Variadic_union's user-defined > destructor (necessary for maintaining trivial destructibility of the > variant iff all of its alternatives are) effectively require a template > instantiation depth of 3x the number of variants, with the instantiation > stack looking like > > ... > _Variadic_union<B, C, ...> > std::is_trivially_destructible_v<_Variadic_union<B, C, ...>> > _Variadic_union<A, B, C, ...>::~_Variadic_union() > _Variadic_union<A, B, C, ...> > ... > > Ideally the template depth should be ~equal to the number of variants > (plus a constant). Luckily it seems we don't need to compute trivial > destructibility of the alternatives at all from _Variadic_union, since > its only user _Variant_storage already has that information. To that > end this patch removes these recursive constraints and instead passes > this information down from _Variant_storage. After this patch, the > template instantiation depth for 87619.cc is ~270 instead of ~780. Perhaps we should also test this change with by setting -ftemplate-depth to something between 256 and 512: libstdc++-v3/testsuite/20_util/variant/87619.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/libstdc++-v3/testsuite/20_util/variant/87619.cc b/libstdc++-v3/testsuite/20_util/variant/87619.cc index 45418e16ca8..b7cfc20858a 100644 --- a/libstdc++-v3/testsuite/20_util/variant/87619.cc +++ b/libstdc++-v3/testsuite/20_util/variant/87619.cc @@ -16,6 +16,7 @@ // <http://www.gnu.org/licenses/>. // { dg-do compile { target c++17 } } +// { dg-additional-options "-ftemplate-depth=300" } #include <variant> #include <utility> > > libstdc++-v3/ChangeLog: > > * include/std/variant (__detail::__variant::_Variadic_union): > Add bool __trivially_destructible template parameter. > (__detail::__variant::_Variadic_union::~_Variadic_union): > Use __trivially_destructible in constraints instead. > (_Variant_storage): Pass __trivially_destructible value to > _Variadic_union. > --- > libstdc++-v3/include/std/variant | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > index 20a76c8aa87..4b9002e0917 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -392,7 +392,7 @@ namespace __variant > }; > > // Defines members and ctors. > - template<typename... _Types> > + template<bool __trivially_destructible, typename... _Types> > union _Variadic_union > { > _Variadic_union() = default; > @@ -401,8 +401,8 @@ namespace __variant > _Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete; > }; > > - template<typename _First, typename... _Rest> > - union _Variadic_union<_First, _Rest...> > + template<bool __trivially_destructible, typename _First, typename... _Rest> > + union _Variadic_union<__trivially_destructible, _First, _Rest...> > { > constexpr _Variadic_union() : _M_rest() { } > > @@ -427,13 +427,12 @@ namespace __variant > ~_Variadic_union() = default; > > constexpr ~_Variadic_union() > - requires (!is_trivially_destructible_v<_First>) > - || (!is_trivially_destructible_v<_Variadic_union<_Rest...>>) > + requires (!__trivially_destructible) > { } > #endif > > _Uninitialized<_First> _M_first; > - _Variadic_union<_Rest...> _M_rest; > + _Variadic_union<__trivially_destructible, _Rest...> _M_rest; > }; > > // _Never_valueless_alt is true for variant alternatives that can > @@ -514,7 +513,7 @@ namespace __variant > return this->_M_index != __index_type(variant_npos); > } > > - _Variadic_union<_Types...> _M_u; > + _Variadic_union<false, _Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; > }; > @@ -552,7 +551,7 @@ namespace __variant > return this->_M_index != static_cast<__index_type>(variant_npos); > } > > - _Variadic_union<_Types...> _M_u; > + _Variadic_union<true, _Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; > }; > -- > 2.43.0.254.ga26002b628 > >
On Sun, Jan 7, 2024 at 3:33 PM Patrick Palka <ppalka@redhat.com> wrote: > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? Ping. > > -- >8 -- > > The recursively defined constraints on _Variadic_union's user-defined > destructor (necessary for maintaining trivial destructibility of the > variant iff all of its alternatives are) effectively require a template > instantiation depth of 3x the number of variants, with the instantiation > stack looking like > > ... > _Variadic_union<B, C, ...> > std::is_trivially_destructible_v<_Variadic_union<B, C, ...>> > _Variadic_union<A, B, C, ...>::~_Variadic_union() > _Variadic_union<A, B, C, ...> > ... > > Ideally the template depth should be ~equal to the number of variants > (plus a constant). Luckily it seems we don't need to compute trivial > destructibility of the alternatives at all from _Variadic_union, since > its only user _Variant_storage already has that information. To that > end this patch removes these recursive constraints and instead passes > this information down from _Variant_storage. After this patch, the > template instantiation depth for 87619.cc is ~270 instead of ~780. > > libstdc++-v3/ChangeLog: > > * include/std/variant (__detail::__variant::_Variadic_union): > Add bool __trivially_destructible template parameter. > (__detail::__variant::_Variadic_union::~_Variadic_union): > Use __trivially_destructible in constraints instead. > (_Variant_storage): Pass __trivially_destructible value to > _Variadic_union. > --- > libstdc++-v3/include/std/variant | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > index 20a76c8aa87..4b9002e0917 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -392,7 +392,7 @@ namespace __variant > }; > > // Defines members and ctors. > - template<typename... _Types> > + template<bool __trivially_destructible, typename... _Types> > union _Variadic_union > { > _Variadic_union() = default; > @@ -401,8 +401,8 @@ namespace __variant > _Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete; > }; > > - template<typename _First, typename... _Rest> > - union _Variadic_union<_First, _Rest...> > + template<bool __trivially_destructible, typename _First, typename... _Rest> > + union _Variadic_union<__trivially_destructible, _First, _Rest...> > { > constexpr _Variadic_union() : _M_rest() { } > > @@ -427,13 +427,12 @@ namespace __variant > ~_Variadic_union() = default; > > constexpr ~_Variadic_union() > - requires (!is_trivially_destructible_v<_First>) > - || (!is_trivially_destructible_v<_Variadic_union<_Rest...>>) > + requires (!__trivially_destructible) > { } > #endif > > _Uninitialized<_First> _M_first; > - _Variadic_union<_Rest...> _M_rest; > + _Variadic_union<__trivially_destructible, _Rest...> _M_rest; > }; > > // _Never_valueless_alt is true for variant alternatives that can > @@ -514,7 +513,7 @@ namespace __variant > return this->_M_index != __index_type(variant_npos); > } > > - _Variadic_union<_Types...> _M_u; > + _Variadic_union<false, _Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; > }; > @@ -552,7 +551,7 @@ namespace __variant > return this->_M_index != static_cast<__index_type>(variant_npos); > } > > - _Variadic_union<_Types...> _M_u; > + _Variadic_union<true, _Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; > }; > -- > 2.43.0.254.ga26002b628 >
On Mon, 15 Jan 2024 at 19:32, Patrick Palka <ppalka@redhat.com> wrote: > > On Sun, Jan 7, 2024 at 3:33 PM Patrick Palka <ppalka@redhat.com> wrote: > > > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > Ping. Huh, I thought I'd already approved this ... sorry. OK for trunk, with the -ftemplate-depth test change too. > > > > > -- >8 -- > > > > The recursively defined constraints on _Variadic_union's user-defined > > destructor (necessary for maintaining trivial destructibility of the > > variant iff all of its alternatives are) effectively require a template > > instantiation depth of 3x the number of variants, with the instantiation > > stack looking like > > > > ... > > _Variadic_union<B, C, ...> > > std::is_trivially_destructible_v<_Variadic_union<B, C, ...>> > > _Variadic_union<A, B, C, ...>::~_Variadic_union() > > _Variadic_union<A, B, C, ...> > > ... > > > > Ideally the template depth should be ~equal to the number of variants > > (plus a constant). Luckily it seems we don't need to compute trivial > > destructibility of the alternatives at all from _Variadic_union, since > > its only user _Variant_storage already has that information. To that > > end this patch removes these recursive constraints and instead passes > > this information down from _Variant_storage. After this patch, the > > template instantiation depth for 87619.cc is ~270 instead of ~780. > > > > libstdc++-v3/ChangeLog: > > > > * include/std/variant (__detail::__variant::_Variadic_union): > > Add bool __trivially_destructible template parameter. > > (__detail::__variant::_Variadic_union::~_Variadic_union): > > Use __trivially_destructible in constraints instead. > > (_Variant_storage): Pass __trivially_destructible value to > > _Variadic_union. > > --- > > libstdc++-v3/include/std/variant | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > > index 20a76c8aa87..4b9002e0917 100644 > > --- a/libstdc++-v3/include/std/variant > > +++ b/libstdc++-v3/include/std/variant > > @@ -392,7 +392,7 @@ namespace __variant > > }; > > > > // Defines members and ctors. > > - template<typename... _Types> > > + template<bool __trivially_destructible, typename... _Types> > > union _Variadic_union > > { > > _Variadic_union() = default; > > @@ -401,8 +401,8 @@ namespace __variant > > _Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete; > > }; > > > > - template<typename _First, typename... _Rest> > > - union _Variadic_union<_First, _Rest...> > > + template<bool __trivially_destructible, typename _First, typename... _Rest> > > + union _Variadic_union<__trivially_destructible, _First, _Rest...> > > { > > constexpr _Variadic_union() : _M_rest() { } > > > > @@ -427,13 +427,12 @@ namespace __variant > > ~_Variadic_union() = default; > > > > constexpr ~_Variadic_union() > > - requires (!is_trivially_destructible_v<_First>) > > - || (!is_trivially_destructible_v<_Variadic_union<_Rest...>>) > > + requires (!__trivially_destructible) > > { } > > #endif > > > > _Uninitialized<_First> _M_first; > > - _Variadic_union<_Rest...> _M_rest; > > + _Variadic_union<__trivially_destructible, _Rest...> _M_rest; > > }; > > > > // _Never_valueless_alt is true for variant alternatives that can > > @@ -514,7 +513,7 @@ namespace __variant > > return this->_M_index != __index_type(variant_npos); > > } > > > > - _Variadic_union<_Types...> _M_u; > > + _Variadic_union<false, _Types...> _M_u; > > using __index_type = __select_index<_Types...>; > > __index_type _M_index; > > }; > > @@ -552,7 +551,7 @@ namespace __variant > > return this->_M_index != static_cast<__index_type>(variant_npos); > > } > > > > - _Variadic_union<_Types...> _M_u; > > + _Variadic_union<true, _Types...> _M_u; > > using __index_type = __select_index<_Types...>; > > __index_type _M_index; > > }; > > -- > > 2.43.0.254.ga26002b628 > > >
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 20a76c8aa87..4b9002e0917 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -392,7 +392,7 @@ namespace __variant }; // Defines members and ctors. - template<typename... _Types> + template<bool __trivially_destructible, typename... _Types> union _Variadic_union { _Variadic_union() = default; @@ -401,8 +401,8 @@ namespace __variant _Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete; }; - template<typename _First, typename... _Rest> - union _Variadic_union<_First, _Rest...> + template<bool __trivially_destructible, typename _First, typename... _Rest> + union _Variadic_union<__trivially_destructible, _First, _Rest...> { constexpr _Variadic_union() : _M_rest() { } @@ -427,13 +427,12 @@ namespace __variant ~_Variadic_union() = default; constexpr ~_Variadic_union() - requires (!is_trivially_destructible_v<_First>) - || (!is_trivially_destructible_v<_Variadic_union<_Rest...>>) + requires (!__trivially_destructible) { } #endif _Uninitialized<_First> _M_first; - _Variadic_union<_Rest...> _M_rest; + _Variadic_union<__trivially_destructible, _Rest...> _M_rest; }; // _Never_valueless_alt is true for variant alternatives that can @@ -514,7 +513,7 @@ namespace __variant return this->_M_index != __index_type(variant_npos); } - _Variadic_union<_Types...> _M_u; + _Variadic_union<false, _Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; }; @@ -552,7 +551,7 @@ namespace __variant return this->_M_index != static_cast<__index_type>(variant_npos); } - _Variadic_union<_Types...> _M_u; + _Variadic_union<true, _Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; };