On Mon, 1 Sep 2025, Tomasz Kaminski wrote:
>
>
> On Fri, Aug 29, 2025 at 5:23 PM Tomasz Kaminski <[email protected]> wrote:
>
> On Fri, Aug 29, 2025 at 5:21 PM Tomasz Kaminski <[email protected]>
> wrote:
>
>
> On Fri, Aug 29, 2025 at 5:16 PM Patrick Palka <[email protected]> wrote:
> On Fri, 29 Aug 2025, Tomasz Kamiński wrote:
>
> > The changes the _Variadic_union implementation, in a way that the
> > _Unitialized<T, false> partial specialization for non-trivial types
> is not
> > necessary.
> >
> > This is simply done by separating the specialization for
> __trivially_destructible
> > being true and false, and for the later definining an empty
> destructor (similary
> > as it was done using concepts).
> >
> > We also reduce the number of specialization of _Varidic_union, so
> specialization
> > (int, int) is reused by (string, int, int) and (int,int). This is
> done by computing
> > initializating __trivially_destructible with conjuction of
> is_trivially_destructible_v
> > for remaining components. This is only necessary for non-trivial
> (false) specialization,
> > as if both _First and _Rest... are trivally destructible, then _Rest
> must also be.
> >
> > This also add test for PR112591.
> > ---
> > As in title, this is refernce for comments, espcially if we want to
> land this change,
> > or part of it to the trunk regardless of other changes. I think the
> test is valuable for sure,
> > but the separate specialization, and removal of
> _Variadic_union<false, int, int>
> > specialization seem usefull.
> >
> > Locally I have removed the _Unitialized<T, false> specialization, and
> the objects
> > no longer alias in C++17 mode (112591.cc works the same in C++20/17),
> and all
> > test have passed locally, so this seem correct. We could even remove
> the _Unitialized
> > altogether and just store _First direclty. This is however ABI break,
> as it does
> > affect layout of classes similar to one in the example.
> >
> >
> > libstdc++-v3/include/std/variant | 34
> ++++++++++++++-----
> > .../testsuite/20_util/variant/112591.cc | 32 +++++++++++++++++
> > 2 files changed, 57 insertions(+), 9 deletions(-)
> > create mode 100644 libstdc++-v3/testsuite/20_util/variant/112591.cc
> >
> > diff --git a/libstdc++-v3/include/std/variant
> b/libstdc++-v3/include/std/variant
> > index 2f44f970028..f2f55837461 100644
> > --- a/libstdc++-v3/include/std/variant
> > +++ b/libstdc++-v3/include/std/variant
> > @@ -393,8 +393,29 @@ namespace __variant
> > _Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete;
> > };
> >
> > - template<bool __trivially_destructible, typename _First,
> typename... _Rest>
> > - union _Variadic_union<__trivially_destructible, _First, _Rest...>
> > + template<typename _First, typename... _Rest>
> > + union _Variadic_union<true, _First, _Rest...>
> > + {
> > + constexpr _Variadic_union() : _M_rest() { }
> > +
> > + template<typename... _Args>
> > + constexpr
> > + _Variadic_union(in_place_index_t<0>, _Args&&... __args)
> > + : _M_first(in_place_index<0>, std::forward<_Args>(__args)...)
> > + { }
> > +
> > + template<size_t _Np, typename... _Args>
> > + constexpr
> > + _Variadic_union(in_place_index_t<_Np>, _Args&&... __args)
> > + : _M_rest(in_place_index<_Np-1>, std::forward<_Args>(__args)...)
> > + { }
> > +
> > + _Uninitialized<_First> _M_first;
> > + _Variadic_union<true, _Rest...> _M_rest;
> > + };
> > +
> > + template<typename _First, typename... _Rest>
> > + union _Variadic_union<false, _First, _Rest...>
> > {
> > constexpr _Variadic_union() : _M_rest() { }
> >
> > @@ -410,24 +431,19 @@ namespace __variant
> > : _M_rest(in_place_index<_Np-1>, std::forward<_Args>(__args)...)
> > { }
> >
> > -#if __cpp_lib_variant >= 202106L
> > _Variadic_union(const _Variadic_union&) = default;
> > _Variadic_union(_Variadic_union&&) = default;
> > _Variadic_union& operator=(const _Variadic_union&) = default;
> > _Variadic_union& operator=(_Variadic_union&&) = default;
> >
> > - ~_Variadic_union() = default;
> > -
> > // If any alternative type is not trivially destructible then
> we need a
> > // user-provided destructor that does nothing. The active
> alternative
> > // will be destroyed by _Variant_storage::_M_reset() instead
> of here.
> > - constexpr ~_Variadic_union()
> > - requires (!__trivially_destructible)
> > + _GLIBCXX20_CONSTEXPR ~_Variadic_union()
> > { }
> > -#endif
> >
> > _Uninitialized<_First> _M_first;
> > - _Variadic_union<__trivially_destructible, _Rest...> _M_rest;
> > + _Variadic_union<(is_trivially_destructible_v<_Rest> && ...),
> _Rest...> _M_rest;
>
> I believe this makes _Variadic_union instantiation quadratic with
> respect to the number of alternatives. How compile-time/memory
> usage fare for the 87619.cc test with this change? (can use
> -ftime-report to measure that)
>
> I have adjusted the test to have a non-trivial destructor, so we hit this
> specialization.
> Results after:
> Time variable wall GGC
> phase setup : 0.00 ( 0%) 2015k ( 1%)
> phase parsing : 0.06 ( 1%) 14M ( 6%)
> phase lang. deferred : 0.56 ( 12%) 152M ( 65%)
> phase opt and generate : 4.20 ( 87%) 66M ( 28%)
> |name lookup : 0.01 ( 0%) 1884k ( 1%)
> |overload resolution : 0.10 ( 2%) 25M ( 11%)
> garbage collection : 0.11 ( 2%) 0 ( 0%)
> callgraph construction : 0.12 ( 2%) 1377k ( 1%)
> callgraph ipa passes : 0.04 ( 1%) 51k ( 0%)
> preprocessing : 0.01 ( 0%) 1044k ( 0%)
> parser (global) : 0.02 ( 0%) 10M ( 4%)
> parser struct body : 0.02 ( 0%) 4272k ( 2%)
> parser inl. meth. body : 0.01 ( 0%) 1392k ( 1%)
> template instantiation : 0.40 ( 8%) 134M ( 57%)
> constant expression evaluation : 0.05 ( 1%) 41k ( 0%)
> symout : 4.08 ( 85%) 80M ( 34%)
> TOTAL : 4.82 235M
>
> Results if I use hardcoded false instead of above && condition:
> Time variable wall GGC
> phase setup : 0.00 ( 0%) 2015k ( 1%)
> phase parsing : 0.06 ( 1%) 14M ( 7%)
> phase lang. deferred : 0.42 ( 9%) 116M ( 58%)
> phase opt and generate : 4.21 ( 90%) 66M ( 33%)
> |name lookup : 0.01 ( 0%) 1884k ( 1%)
> |overload resolution : 0.02 ( 0%) 3277k ( 2%)
> garbage collection : 0.10 ( 2%) 0 ( 0%)
> callgraph construction : 0.12 ( 2%) 1377k ( 1%)
> callgraph ipa passes : 0.03 ( 1%) 51k ( 0%)
> preprocessing : 0.01 ( 0%) 1044k ( 1%)
> parser (global) : 0.02 ( 0%) 10M ( 5%)
> parser struct body : 0.01 ( 0%) 4272k ( 2%)
> parser inl. meth. body : 0.01 ( 0%) 1392k ( 1%)
> template instantiation : 0.30 ( 6%) 98M ( 49%)
> constant expression evaluation : 0.01 ( 0%) 41k ( 0%)
> symout : 4.10 ( 87%) 80M ( 40%)
> TOTAL : 4.69 199M
>
> Template depth in 267 in both cases.
>
> Does the above look acceptable to you? In less syntetic examples they may be
> some
> benefits from reducing the number of template instantiations.
Makes sense, I'm fine with it.
>
>
>
> > };
> >
> > // _Never_valueless_alt is true for variant alternatives that can
> > diff --git a/libstdc++-v3/testsuite/20_util/variant/112591.cc
> b/libstdc++-v3/testsuite/20_util/variant/112591.cc
> > new file mode 100644
> > index 00000000000..b1b07c4f46e
> > --- /dev/null
> > +++ b/libstdc++-v3/testsuite/20_util/variant/112591.cc
> > @@ -0,0 +1,32 @@
> > +// { dg-do run { target c++17 } }
> > +
> > +#include <variant>
> > +#include <testsuite_hooks.h>
> > +
> > +struct NonEmpty { int x; };
> > +struct TrivialEmpty {};
> > +struct NonTrivialEmpty { ~NonTrivialEmpty() {} };
> > +
> > +template<typename T>
> > +struct Compose : T
> > +{
> > + std::variant<T, int> v;
> > +};
> > +
> > +template<typename T>
> > +bool testAlias()
> > +{
> > + Compose<T> c;
> > + return static_cast<T*>(&c) == &std::get<T>(c.v);
> > +}
> > +
> > +int main()
> > +{
> > + VERIFY( !testAlias<NonEmpty>() );
> > + VERIFY( !testAlias<TrivialEmpty>() );
> > +#if __cplusplus >= 202002L
> > + VERIFY( !testAlias<NonTrivialEmpty>() );
> > +#else
> > + VERIFY( testAlias<NonTrivialEmpty>() );
> > +#endif
> > +}
> > --
> > 2.50.1
> >
> >
>
>
>
