On Fri, Aug 29, 2025 at 5:23 PM Tomasz Kaminski <tkami...@redhat.com> wrote:
>
> On Fri, Aug 29, 2025 at 5:21 PM Tomasz Kaminski <tkami...@redhat.com>
> wrote:
>
>>
>>
>> On Fri, Aug 29, 2025 at 5:16 PM Patrick Palka <ppa...@redhat.com> 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.
>
>>
>>
>>>
>>> > };
>>> >
>>> > // _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
>>> >
>>> >
>>
>>
