[libc++] Support constexpr for std::stable_sort in radix sort branch
#125284
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Дмитрий Изволов (izvolov) Changes
#119394 Full diff: https://github.com/llvm/llvm-project/pull/125284.diff 3 Files Affected:
diff --git a/libcxx/include/__algorithm/radix_sort.h b/libcxx/include/__algorithm/radix_sort.h
index de6927995e74c8..89df4e19fd00fb 100644
--- a/libcxx/include/__algorithm/radix_sort.h
+++ b/libcxx/include/__algorithm/radix_sort.h
@@ -67,7 +67,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
#if _LIBCPP_STD_VER >= 14
template <class _InputIterator, class _OutputIterator>
-_LIBCPP_HIDE_FROM_ABI pair<_OutputIterator, __iter_value_type<_InputIterator>>
+_LIBCPP_HIDE_FROM_ABI constexpr pair<_OutputIterator, __iter_value_type<_InputIterator>>
__partial_sum_max(_InputIterator __first, _InputIterator __last, _OutputIterator __result) {
if (__first == __last)
return {__result, 0};
@@ -109,7 +109,7 @@ struct __counting_sort_traits {
};
template <class _Radix, class _Integer>
-_LIBCPP_HIDE_FROM_ABI auto __nth_radix(size_t __radix_number, _Radix __radix, _Integer __n) {
+_LIBCPP_HIDE_FROM_ABI constexpr auto __nth_radix(size_t __radix_number, _Radix __radix, _Integer __n) {
static_assert(is_unsigned<_Integer>::value);
using __traits = __counting_sort_traits<_Integer, _Radix>;
@@ -117,7 +117,7 @@ _LIBCPP_HIDE_FROM_ABI auto __nth_radix(size_t __radix_number, _Radix __radix, _I
}
template <class _ForwardIterator, class _Map, class _RandomAccessIterator>
-_LIBCPP_HIDE_FROM_ABI void
+_LIBCPP_HIDE_FROM_ABI constexpr void
__collect(_ForwardIterator __first, _ForwardIterator __last, _Map __map, _RandomAccessIterator __counters) {
using __value_type = __iter_value_type<_ForwardIterator>;
using __traits = __counting_sort_traits<__value_type, _Map>;
@@ -129,7 +129,7 @@ __collect(_ForwardIterator __first, _ForwardIterator __last, _Map __map, _Random
}
template <class _ForwardIterator, class _RandomAccessIterator1, class _Map, class _RandomAccessIterator2>
-_LIBCPP_HIDE_FROM_ABI void
+_LIBCPP_HIDE_FROM_ABI constexpr void
__dispose(_ForwardIterator __first,
_ForwardIterator __last,
_RandomAccessIterator1 __result,
@@ -147,7 +147,7 @@ template <class _ForwardIterator,
class _RandomAccessIterator1,
class _RandomAccessIterator2,
size_t... _Radices>
-_LIBCPP_HIDE_FROM_ABI bool __collect_impl(
+_LIBCPP_HIDE_FROM_ABI constexpr bool __collect_impl(
_ForwardIterator __first,
_ForwardIterator __last,
_Map __map,
@@ -177,7 +177,7 @@ _LIBCPP_HIDE_FROM_ABI bool __collect_impl(
}
template <class _ForwardIterator, class _Map, class _Radix, class _RandomAccessIterator1, class _RandomAccessIterator2>
-_LIBCPP_HIDE_FROM_ABI bool
+_LIBCPP_HIDE_FROM_ABI constexpr bool
__collect(_ForwardIterator __first,
_ForwardIterator __last,
_Map __map,
@@ -191,7 +191,7 @@ __collect(_ForwardIterator __first,
}
template <class _BidirectionalIterator, class _RandomAccessIterator1, class _Map, class _RandomAccessIterator2>
-_LIBCPP_HIDE_FROM_ABI void __dispose_backward(
+_LIBCPP_HIDE_FROM_ABI constexpr void __dispose_backward(
_BidirectionalIterator __first,
_BidirectionalIterator __last,
_RandomAccessIterator1 __result,
@@ -206,7 +206,7 @@ _LIBCPP_HIDE_FROM_ABI void __dispose_backward(
}
template <class _ForwardIterator, class _RandomAccessIterator, class _Map>
-_LIBCPP_HIDE_FROM_ABI _RandomAccessIterator
+_LIBCPP_HIDE_FROM_ABI constexpr _RandomAccessIterator
__counting_sort_impl(_ForwardIterator __first, _ForwardIterator __last, _RandomAccessIterator __result, _Map __map) {
using __value_type = __iter_value_type<_ForwardIterator>;
using __traits = __counting_sort_traits<__value_type, _Map>;
@@ -225,7 +225,7 @@ template <class _RandomAccessIterator1,
class _Radix,
enable_if_t< __radix_sort_traits<__iter_value_type<_RandomAccessIterator1>, _Map, _Radix>::__radix_count == 1,
int> = 0>
-_LIBCPP_HIDE_FROM_ABI void __radix_sort_impl(
+_LIBCPP_HIDE_FROM_ABI constexpr void __radix_sort_impl(
_RandomAccessIterator1 __first,
_RandomAccessIterator1 __last,
_RandomAccessIterator2 __buffer,
@@ -245,7 +245,7 @@ template <
class _Radix,
enable_if_t< __radix_sort_traits<__iter_value_type<_RandomAccessIterator1>, _Map, _Radix>::__radix_count % 2 == 0,
int> = 0 >
-_LIBCPP_HIDE_FROM_ABI void __radix_sort_impl(
+_LIBCPP_HIDE_FROM_ABI constexpr void __radix_sort_impl(
_RandomAccessIterator1 __first,
_RandomAccessIterator1 __last,
_RandomAccessIterator2 __buffer_begin,
@@ -307,7 +307,7 @@ struct __low_byte_fn {
};
template <class _RandomAccessIterator1, class _RandomAccessIterator2, class _Map, class _Radix>
-_LIBCPP_HIDE_FROM_ABI void
+_LIBCPP_HIDE_FROM_ABI constexpr void
__radix_sort(_RandomAccessIterator1 __first,
_RandomAccessIterator1 __last,
_RandomAccessIterator2 __buffer,
@@ -318,7 +318,7 @@ __radix_sort(_RandomAccessIterator1 __first,
}
template <class _RandomAccessIterator1, class _RandomAccessIterator2>
-_LIBCPP_HIDE_FROM_ABI void
+_LIBCPP_HIDE_FROM_ABI constexpr void
__radix_sort(_RandomAccessIterator1 __first, _RandomAccessIterator1 __last, _RandomAccessIterator2 __buffer) {
std::__radix_sort(__first, __last, __buffer, __identity{}, __low_byte_fn{});
}
diff --git a/libcxx/include/__algorithm/stable_sort.h b/libcxx/include/__algorithm/stable_sort.h
index 3cfbcf08d2c5c4..21bc3a947069c0 100644
--- a/libcxx/include/__algorithm/stable_sort.h
+++ b/libcxx/include/__algorithm/stable_sort.h
@@ -253,6 +253,12 @@ _LIBCPP_CONSTEXPR_SINCE_CXX26 void __stable_sort(
if constexpr (__allowed_radix_sort) {
if (__len <= __buff_size && __len >= static_cast<difference_type>(__radix_sort_min_bound<value_type>()) &&
__len <= static_cast<difference_type>(__radix_sort_max_bound<value_type>())) {
+ for (auto* p = __buff; p < __buff + __buff_size; ++p) {
+ std::__construct_at(p, 0);
+ }
+ __destruct_n __d(__buff_size);
+ unique_ptr<value_type, __destruct_n&> __h2(__buff, __d);
+
std::__radix_sort(__first, __last, __buff);
return;
}
diff --git a/libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/stable_sort.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/stable_sort.pass.cpp
index b3b458808c44ad..1e529529b78f96 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/stable_sort.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/stable_sort.pass.cpp
@@ -178,8 +178,9 @@ TEST_CONSTEXPR_CXX26 bool test() {
test_larger_sorts<T>(1009);
test_larger_sorts<T>(1024);
test_larger_sorts<T>(1031);
- test_larger_sorts<T>(2053);
}
+ // test constexprness of radix sort branch
+ test_larger_sorts<T>(2053);
// check that the algorithm works without memory
#ifndef TEST_HAS_NO_EXCEPTIONS
|
izvolov
commented
Jan 31, 2025
| @@ -253,6 +253,12 @@ _LIBCPP_CONSTEXPR_SINCE_CXX26 void __stable_sort( | |||
| if constexpr (__allowed_radix_sort) { | |||
| if (__len <= __buff_size && __len >= static_cast<difference_type>(__radix_sort_min_bound<value_type>()) && | |||
| __len <= static_cast<difference_type>(__radix_sort_max_bound<value_type>())) { | |||
| for (auto* p = __buff; p < __buff + __buff_size; ++p) { | |||
There was a problem hiding this comment.
This looks disgusting, but taking __construct_at inside radix_sort would degrade the design of the algorithm.
Maybe the better solution exists?
|
@philnik777 @ldionne @PaulXiCao |
philnik777
reviewed
Feb 3, 2025
Comment on lines
256
to
260
| for (auto* __p = __buff; __p < __buff + __buff_size; ++__p) { | ||
| std::__construct_at(__p, 0); | ||
| } | ||
| __destruct_n __d(__buff_size); | ||
| unique_ptr<value_type, __destruct_n&> __h2(__buff, __d); |
There was a problem hiding this comment.
This is all only required for constexpr support, right? I'm pretty sure you can drop the destruction, since the elements should always be trivial. For the construction, I think we don't need the zero and we should probably only do it during constant evaluation.
There was a problem hiding this comment.
Yes, you're right.
Done all three things.
|
@izvolov Did you forget to upload the update? |
izvolov
force-pushed
the
constexpr-radix-sort
branch
from
790a028 to
08a6649
Compare
Hm, github didn't catch changes from my fork for some reason. |
philnik777
approved these changes
Feb 4, 2025
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…nch (llvm#125284) `std::stable_sort` is `constexpr` since PR llvm#110320 But `radix_sort` branch is still non-`constexpr`. This PR fixes it. llvm#119394 llvm#105360
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
std::stable_sortisconstexprsince PR #110320But
radix_sortbranch is still non-constexpr.This PR fixes it.
#119394
#105360