-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libc++][NFC] Reuse __bit_log2 for sort
#135303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libc++][NFC] Reuse __bit_log2 for sort
#135303
Conversation
The `__log2i` function template in `<algorithm/sort.h>` is basically equivalent to `__bit_log2` in `<__bit/bit_log2.h>`. It seems better to avoid duplication.
|
@llvm/pr-subscribers-libcxx Author: A. Jiang (frederick-vs-ja) ChangesThe Full diff: https://github.com/llvm/llvm-project/pull/135303.diff 2 Files Affected:
diff --git a/libcxx/include/__algorithm/sort.h b/libcxx/include/__algorithm/sort.h
index 4332b62544b40..5ffe40527c0d7 100644
--- a/libcxx/include/__algorithm/sort.h
+++ b/libcxx/include/__algorithm/sort.h
@@ -17,6 +17,7 @@
#include <__algorithm/partial_sort.h>
#include <__algorithm/unwrap_iter.h>
#include <__assert>
+#include <__bit/bit_log2.h>
#include <__bit/blsr.h>
#include <__bit/countl.h>
#include <__bit/countr.h>
@@ -34,6 +35,7 @@
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_trivially_copyable.h>
+#include <__type_traits/make_unsigned.h>
#include <__utility/move.h>
#include <__utility/pair.h>
#include <climits>
@@ -826,25 +828,6 @@ void __introsort(_RandomAccessIterator __first,
}
}
-template <typename _Number>
-inline _LIBCPP_HIDE_FROM_ABI _Number __log2i(_Number __n) {
- if (__n == 0)
- return 0;
- if (sizeof(__n) <= sizeof(unsigned))
- return sizeof(unsigned) * CHAR_BIT - 1 - __libcpp_clz(static_cast<unsigned>(__n));
- if (sizeof(__n) <= sizeof(unsigned long))
- return sizeof(unsigned long) * CHAR_BIT - 1 - __libcpp_clz(static_cast<unsigned long>(__n));
- if (sizeof(__n) <= sizeof(unsigned long long))
- return sizeof(unsigned long long) * CHAR_BIT - 1 - __libcpp_clz(static_cast<unsigned long long>(__n));
-
- _Number __log2 = 0;
- while (__n > 1) {
- __log2++;
- __n >>= 1;
- }
- return __log2;
-}
-
template <class _Comp, class _RandomAccessIterator>
void __sort(_RandomAccessIterator, _RandomAccessIterator, _Comp);
@@ -878,7 +861,8 @@ template <class _AlgPolicy, class _RandomAccessIterator, class _Comp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
__sort_dispatch(_RandomAccessIterator __first, _RandomAccessIterator __last, _Comp& __comp) {
typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
- difference_type __depth_limit = 2 * std::__log2i(__last - __first);
+ difference_type __depth_limit = static_cast<difference_type>(
+ 2 * std::__bit_log2(static_cast<__make_unsigned_t<difference_type> >(__last - __first)));
// Only use bitset partitioning for arithmetic types. We should also check
// that the default comparator is in use so that we are sure that there are no
diff --git a/libcxx/include/__bit/bit_log2.h b/libcxx/include/__bit/bit_log2.h
index 94ee6c3b2bb1d..b22e1ce1f84e6 100644
--- a/libcxx/include/__bit/bit_log2.h
+++ b/libcxx/include/__bit/bit_log2.h
@@ -20,16 +20,12 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-#if _LIBCPP_STD_VER >= 14
-
template <class _Tp>
-_LIBCPP_HIDE_FROM_ABI constexpr _Tp __bit_log2(_Tp __t) noexcept {
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __bit_log2(_Tp __t) _NOEXCEPT {
static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__bit_log2 requires an unsigned integer type");
return numeric_limits<_Tp>::digits - 1 - std::__countl_zero(__t);
}
-#endif // _LIBCPP_STD_VER >= 14
-
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___BIT_BIT_LOG2_H
|
philnik777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming the CI is happy.
In llvm#135303, we started using __bit_log2 instead of __log2i inside `std::sort`. However, __bit_log2 has a precondition that __log2i didn't have, which is that the input is non-zero. While it technically makes no sense to request the logarihm of 0, __log2i handled that case and returned 0 without issues. After switching to __bit_log2, passing 0 as an input results in an unsigned integer overflow which can trigger -fsanitize=unsigned-integer-overflow. While not technically UB in itself, it's clearly not intended either. To fix this, we add an internal assertion to __bit_log2 which catches the issue in our test suite, and we make sure not to violate __bit_log2's preconditions before we call it from `std::sort`.
In #135303, we started using `__bit_log2` instead of `__log2i` inside `std::sort`. However, `__bit_log2` has a precondition that `__log2i` didn't have, which is that the input is non-zero. While it technically makes no sense to request the logarithm of 0, `__log2i` handled that case and returned 0 without issues. After switching to `__bit_log2`, passing 0 as an input results in an unsigned integer overflow which can trigger `-fsanitize=unsigned-integer-overflow`. While not technically UB in itself, it's clearly not intended either. To fix this, we add an internal assertion to `__bit_log2` which catches the issue in our test suite, and we make sure not to violate `__bit_log2`'s preconditions before we call it from `std::sort`.
In llvm#135303, we started using `__bit_log2` instead of `__log2i` inside `std::sort`. However, `__bit_log2` has a precondition that `__log2i` didn't have, which is that the input is non-zero. While it technically makes no sense to request the logarithm of 0, `__log2i` handled that case and returned 0 without issues. After switching to `__bit_log2`, passing 0 as an input results in an unsigned integer overflow which can trigger `-fsanitize=unsigned-integer-overflow`. While not technically UB in itself, it's clearly not intended either. To fix this, we add an internal assertion to `__bit_log2` which catches the issue in our test suite, and we make sure not to violate `__bit_log2`'s preconditions before we call it from `std::sort`. (cherry picked from commit 2ae4b92)
The
__log2ifunction template in<algorithm/sort.h>is basically equivalent to__bit_log2in<__bit/bit_log2.h>. It seems better to avoid duplication.