[2/6] libstdc++: pstl: port away from is_trivial

For PSTL, I've just chosen to port from is_trivial to the direct 
equivalent, as I'm really unsure about the meaning of the code.

On 09/12/24 13:08 +0100, Giuseppe D'Angelo wrote:
>For PSTL, I've just chosen to port from is_trivial to the direct 
>equivalent, as I'm really unsure about the meaning of the code.

Yeah, I'm not sure either.

I note that the code uses std::conjunction instead of the non-portable
__and_. std::conjunction is only available in C++17 and upstream PSTL
wants to be valid as C++14 too (and maybe even C++11). But upstream
doesn't want to use our non-portable __and_ either, so we're in a bit
of a bind.

The patch is fine as-is, but it's something we might want to revisit
in future.

Could I ask you to open an issue about std::is_trivial upstream:
https://github.com/oneapi-src/oneDPL/issues
If/when they fix it, we can consider aligning with their direction.
And that might resolve the question of which aspects of triviality are
really needed here.


>-- 
>Giuseppe D'Angelo

>From a24a9495976a08f19df32dddf9b0e29661aca696 Mon Sep 17 00:00:00 2001
>From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
>Date: Mon, 9 Dec 2024 02:06:25 +0100
>Subject: [PATCH 2/6] libstdc++: pstl: port away from is_trivial
>
>In preparation for the deprecation of is_trivial (P3247R2).
>Unfortunately I am unable to fully understand what aspect of triviality
>seems to matter for these algorithms, so I just ported is_trivial to its
>direct equivalent (trivially copyable + trivially default
>constructible.)
>
>libstdc++-v3/ChangeLog:
>
>	* include/pstl/algorithm_impl.h (__remove_elements): Port away
>	  from is_trivial.
>	(__pattern_inplace_merge): Likewise.
>	* include/pstl/glue_memory_impl.h (uninitialized_copy): Likewise.
>	(uninitialized_copy_n): Likewise.
>	(uninitialized_move): Likewise.
>	(uninitialized_move_n): Likewise.
>	(uninitialized_default_construct): Likewise.
>	(uninitialized_default_construct_n): Likewise.
>	(uninitialized_value_construct): Likewise.
>	(uninitialized_value_construct_n): Likewise.
>	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc: Likewise.
>	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc: Likewise.
>	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc: Likewise.
>	* testsuite/25_algorithms/pstl/alg_modifying_operations/partition.cc: Likewise.
>
>Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
>---
> libstdc++-v3/include/pstl/algorithm_impl.h    |  9 +++++---
> libstdc++-v3/include/pstl/glue_memory_impl.h  | 21 ++++++++++++-------
> .../pstl/uninitialized_construct.cc           |  2 +-
> .../pstl/uninitialized_copy_move.cc           |  2 +-
> .../pstl/uninitialized_fill_destroy.cc        |  2 +-
> .../alg_modifying_operations/partition.cc     | 13 ++++++++++--
> 6 files changed, 33 insertions(+), 16 deletions(-)
>
>diff --git a/libstdc++-v3/include/pstl/algorithm_impl.h b/libstdc++-v3/include/pstl/algorithm_impl.h
>index 1403b02280f..5b1cd201094 100644
>--- a/libstdc++-v3/include/pstl/algorithm_impl.h
>+++ b/libstdc++-v3/include/pstl/algorithm_impl.h
>@@ -1297,7 +1297,8 @@ __remove_elements(__parallel_tag<_IsVector> __tag, _ExecutionPolicy&& __exec, _F
>                     [](_ForwardIterator __x, _Tp* __z)
>                     {
>                         __internal::__invoke_if_else(
>-                            std::is_trivial<_Tp>(), [&]() { *__z = std::move(*__x); },
>+                            std::conjunction<std::is_trivially_copyable<_Tp>, std::is_trivially_default_constructible<_Tp>>(),
>+                            [&]() { *__z = std::move(*__x); },
>                             [&]() { ::new (std::addressof(*__z)) _Tp(std::move(*__x)); });
>                     },
>                     _IsVector{});
>@@ -1310,7 +1311,8 @@ __remove_elements(__parallel_tag<_IsVector> __tag, _ExecutionPolicy&& __exec, _F
>             [__result, __first](_Tp* __i, _Tp* __j)
>             {
>                 __invoke_if_else(
>-                    std::is_trivial<_Tp>(), [&]() { __brick_move(__i, __j, __first + (__i - __result), _IsVector{}); },
>+                    std::conjunction<std::is_trivially_copyable<_Tp>, std::is_trivially_default_constructible<_Tp>>(),
>+                    [&]() { __brick_move(__i, __j, __first + (__i - __result), _IsVector{}); },
>                     [&]() { __brick_move_destroy()(__i, __j, __first + (__i - __result), _IsVector{}); });
>             });
>         return __first + __m;
>@@ -2794,7 +2796,8 @@ __pattern_inplace_merge(__parallel_tag<_IsVector> __tag, _ExecutionPolicy&& __ex
>             auto __move_values = [](_RandomAccessIterator __x, _Tp* __z)
>             {
>                 __internal::__invoke_if_else(
>-                    std::is_trivial<_Tp>(), [&]() { *__z = std::move(*__x); },
>+                    std::conjunction<std::is_trivially_copyable<_Tp>, std::is_trivially_default_constructible<_Tp>>(),
>+                    [&]() { *__z = std::move(*__x); },
>                     [&]() { ::new (std::addressof(*__z)) _Tp(std::move(*__x)); });
>             };
> 
>diff --git a/libstdc++-v3/include/pstl/glue_memory_impl.h b/libstdc++-v3/include/pstl/glue_memory_impl.h
>index 7850605f94a..0f37fbbf214 100644
>--- a/libstdc++-v3/include/pstl/glue_memory_impl.h
>+++ b/libstdc++-v3/include/pstl/glue_memory_impl.h
>@@ -34,7 +34,8 @@ uninitialized_copy(_ExecutionPolicy&& __exec, _InputIterator __first, _InputIter
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::integral_constant < bool, std::is_trivial<_ValueType1>::value&& std::is_trivial<_ValueType2>::value > (),
>+        std::conjunction<std::is_trivially_copyable<_ValueType1>, std::is_trivially_default_constructible<_ValueType1>,
>+                         std::is_trivially_copyable<_ValueType2>, std::is_trivially_default_constructible<_ValueType2>>(),
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk2_brick(
>@@ -65,7 +66,8 @@ uninitialized_copy_n(_ExecutionPolicy&& __exec, _InputIterator __first, _Size __
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::integral_constant < bool, std::is_trivial<_ValueType1>::value&& std::is_trivial<_ValueType2>::value > (),
>+        std::conjunction<std::is_trivially_copyable<_ValueType1>, std::is_trivially_default_constructible<_ValueType1>,
>+                         std::is_trivially_copyable<_ValueType2>, std::is_trivially_default_constructible<_ValueType2>>(),
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk2_brick_n(
>@@ -98,7 +100,8 @@ uninitialized_move(_ExecutionPolicy&& __exec, _InputIterator __first, _InputIter
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::integral_constant < bool, std::is_trivial<_ValueType1>::value&& std::is_trivial<_ValueType2>::value > (),
>+        std::conjunction<std::is_trivially_copyable<_ValueType1>, std::is_trivially_default_constructible<_ValueType1>,
>+                         std::is_trivially_copyable<_ValueType2>, std::is_trivially_default_constructible<_ValueType2>>(),
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk2_brick(
>@@ -129,7 +132,8 @@ uninitialized_move_n(_ExecutionPolicy&& __exec, _InputIterator __first, _Size __
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::integral_constant < bool, std::is_trivial<_ValueType1>::value&& std::is_trivial<_ValueType2>::value > (),
>+        std::conjunction<std::is_trivially_copyable<_ValueType1>, std::is_trivially_default_constructible<_ValueType1>,
>+                         std::is_trivially_copyable<_ValueType2>, std::is_trivially_default_constructible<_ValueType2>>(),
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk2_brick_n(
>@@ -254,7 +258,7 @@ uninitialized_default_construct(_ExecutionPolicy&& __exec, _ForwardIterator __fi
> 
>     auto __dispatch_tag = __pstl::__internal::__select_backend(__exec, __first);
> 
>-    __pstl::__internal::__invoke_if_not(std::is_trivial<_ValueType>(),
>+    __pstl::__internal::__invoke_if_not(std::conjunction<std::is_trivially_copyable<_ValueType>, std::is_trivially_default_constructible<_ValueType>>(),
>                                         [&]()
>                                         {
>                                             __pstl::__internal::__pattern_walk1(
>@@ -273,7 +277,8 @@ uninitialized_default_construct_n(_ExecutionPolicy&& __exec, _ForwardIterator __
>     auto __dispatch_tag = __pstl::__internal::__select_backend(__exec, __first);
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::is_trivial<_ValueType>(), [&]() { return std::next(__first, __n); },
>+        std::conjunction<std::is_trivially_copyable<_ValueType>, std::is_trivially_default_constructible<_ValueType>>(),
>+        [&]() { return std::next(__first, __n); },
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk1_n(
>@@ -296,7 +301,7 @@ uninitialized_value_construct(_ExecutionPolicy&& __exec, _ForwardIterator __firs
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     __pstl::__internal::__invoke_if_else(
>-        std::is_trivial<_ValueType>(),
>+        std::conjunction<std::is_trivially_copyable<_ValueType>, std::is_trivially_default_constructible<_ValueType>>(),
>         [&]()
>         {
>             __pstl::__internal::__pattern_walk_brick(
>@@ -324,7 +329,7 @@ uninitialized_value_construct_n(_ExecutionPolicy&& __exec, _ForwardIterator __fi
>     using __is_vector = typename decltype(__dispatch_tag)::__is_vector;
> 
>     return __pstl::__internal::__invoke_if_else(
>-        std::is_trivial<_ValueType>(),
>+        std::conjunction<std::is_trivially_copyable<_ValueType>, std::is_trivially_default_constructible<_ValueType>>(),
>         [&]()
>         {
>             return __pstl::__internal::__pattern_walk_brick_n(
>diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc
>index 407e4885327..0bc1e3266d9 100644
>--- a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc
>+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc
>@@ -108,7 +108,7 @@ test_uninit_construct_by_type()
>     for (size_t n = 0; n <= N; n = n <= 16 ? n + 1 : size_t(3.1415 * n))
>     {
>         std::unique_ptr<T[]> p(new T[n]);
>-        invoke_on_all_policies(test_uninit_construct(), p.get(), std::next(p.get(), n), n, std::is_trivial<T>());
>+        invoke_on_all_policies(test_uninit_construct(), p.get(), std::next(p.get(), n), n, std::conjunction<std::is_trivially_copyable<T>, std::is_trivially_default_constructible<T>>());
>     }
> }
> 
>diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc
>index 2558147a7f1..38c1bf136df 100644
>--- a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc
>+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc
>@@ -128,7 +128,7 @@ test_uninitialized_copy_move_by_type()
>     {
>         Sequence<T> in(n, [=](size_t k) -> T { return T(k); });
>         std::unique_ptr<T[]> p(new T[n]);
>-        invoke_on_all_policies(test_uninitialized_copy_move(), in.begin(), in.end(), p.get(), n, std::is_trivial<T>());
>+        invoke_on_all_policies(test_uninitialized_copy_move(), in.begin(), in.end(), p.get(), n, std::conjunction<std::is_trivially_copyable<T>, std::is_trivially_default_constructible<T>>());
>     }
> }
> 
>diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc
>index fed9bdb8886..dfadb554a11 100644
>--- a/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc
>+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc
>@@ -83,7 +83,7 @@ test_uninitialized_fill_destroy_by_type()
>     {
>         std::unique_ptr<T[]> p(new T[n]);
>         invoke_on_all_policies(test_uninitialized_fill_destroy(), p.get(), std::next(p.get(), n), T(), n,
>-                               std::is_trivial<T>());
>+                               std::conjunction<std::is_trivially_copyable<T>, std::is_trivially_default_constructible<T>>());
>     }
> }
> 
>diff --git a/libstdc++-v3/testsuite/25_algorithms/pstl/alg_modifying_operations/partition.cc b/libstdc++-v3/testsuite/25_algorithms/pstl/alg_modifying_operations/partition.cc
>index e13fe938d1f..58979bac2b0 100644
>--- a/libstdc++-v3/testsuite/25_algorithms/pstl/alg_modifying_operations/partition.cc
>+++ b/libstdc++-v3/testsuite/25_algorithms/pstl/alg_modifying_operations/partition.cc
>@@ -57,15 +57,24 @@ struct DataType
>     T my_val;
> };
> 
>+#pragma GCC diagnostic push
>+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>+// is_trivial is deprecated in C++26
> template <typename Iterator>
>-typename std::enable_if<std::is_trivial<typename std::iterator_traits<Iterator>::value_type>::value, bool>::type
>+using iterator_value_type_is_trivial = std::is_trivial<
>+    typename std::iterator_traits<Iterator>::value_type
>+>;
>+#pragma GCC diagnostic pop
>+
>+template <typename Iterator>
>+typename std::enable_if<iterator_value_type_is_trivial<Iterator>::value, bool>::type
> is_equal(Iterator first, Iterator last, Iterator d_first)
> {
>     return std::equal(first, last, d_first);
> }
> 
> template <typename Iterator>
>-typename std::enable_if<!std::is_trivial<typename std::iterator_traits<Iterator>::value_type>::value, bool>::type
>+typename std::enable_if<!iterator_value_type_is_trivial<Iterator>::value, bool>::type
>     is_equal(Iterator, Iterator, Iterator)
> {
>     return true;
>-- 
>2.34.1
>

On 09/12/24 13:13 +0000, Jonathan Wakely wrote:
>On 09/12/24 13:08 +0100, Giuseppe D'Angelo wrote:
>>For PSTL, I've just chosen to port from is_trivial to the direct 
>>equivalent, as I'm really unsure about the meaning of the code.
>
>Yeah, I'm not sure either.
>
>I note that the code uses std::conjunction instead of the non-portable
>__and_. std::conjunction is only available in C++17 and upstream PSTL
>wants to be valid as C++14 too (and maybe even C++11). But upstream
>doesn't want to use our non-portable __and_ either, so we're in a bit
>of a bind.

Actually it looks like upstream are happy to use C++17 features now
and have replace is_trivial<T>::value with is_trivial_v<T>@
https://github.com/oneapi-src/oneDPL/pull/1173
Next time we rebase the PSTL code on upstream, we'll pick that change
up.

It would still be good to make them aware of the is_trivial
deprecation by filing an issue.

On 09/12/2024 14:13, Jonathan Wakely wrote:
> 
> Could I ask you to open an issue about std::is_trivial upstream:
> https://github.com/oneapi-src/oneDPL/issues
> If/when they fix it, we can consider aligning with their direction.
> And that might resolve the question of which aspects of triviality are
> really needed here.

Certainly; I've submitted

https://github.com/oneapi-src/oneDPL/issues/1961

Thank you,