-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Refactor __next_prime to be const #157421
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
base: main
Are you sure you want to change the base?
Conversation
96e46b9
to
7f14d42
Compare
7f14d42
to
31741f5
Compare
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis can improve the codegen a bit, since the compiler can now know that Full diff: https://github.com/llvm/llvm-project/pull/157421.diff 6 Files Affected:
diff --git a/libcxx/include/__configuration/availability.h b/libcxx/include/__configuration/availability.h
index 2fbc34a3cf8a2..e3903b610a3ec 100644
--- a/libcxx/include/__configuration/availability.h
+++ b/libcxx/include/__configuration/availability.h
@@ -84,6 +84,9 @@
// in all versions of the library are available.
#if !_LIBCPP_HAS_VENDOR_AVAILABILITY_ANNOTATIONS
+# define _LIBCPP_INTRODUCED_IN_LLVM_22 1
+# define _LIBCPP_INTRODUCED_IN_LLVM_22_ATTRIBUTE /* nothing */
+
# define _LIBCPP_INTRODUCED_IN_LLVM_21 1
# define _LIBCPP_INTRODUCED_IN_LLVM_21_ATTRIBUTE /* nothing */
@@ -120,6 +123,11 @@
// clang-format off
+// LLVM 22
+// TODO: Fill this in
+# define _LIBCPP_INTRODUCED_IN_LLVM_22 0
+# define _LIBCPP_INTRODUCED_IN_LLVM_22_ATTRIBUTE __attribute__((unavailable))
+
// LLVM 21
// TODO: Fill this in
# define _LIBCPP_INTRODUCED_IN_LLVM_21 0
@@ -355,6 +363,9 @@
#define _LIBCPP_AVAILABILITY_HAS_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE _LIBCPP_INTRODUCED_IN_LLVM_21
// No attribute, since we've had bad_function_call::what() in the headers before
+// This controls whether `std::__next_prime_impl` is available in the dylib, which is used for `__hash_table`.
+#define _LIBCPP_AVAILABILITY_HAS_NEXT_PRIME_IMPL _LIBCPP_INTRODUCED_IN_LLVM_22
+
// Define availability attributes that depend on both
// _LIBCPP_HAS_EXCEPTIONS and _LIBCPP_HAS_RTTI.
#if !_LIBCPP_HAS_EXCEPTIONS || !_LIBCPP_HAS_RTTI
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 91f660d3491e8..49c3c35181452 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -47,6 +47,7 @@
#include <__utility/swap.h>
#include <__utility/try_key_extraction.h>
#include <limits>
+#include <stdexcept>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
@@ -72,7 +73,28 @@ struct __is_hash_value_type : false_type {};
template <class _One>
struct __is_hash_value_type<_One> : __is_hash_value_type_imp<__remove_cvref_t<_One> > {};
-_LIBCPP_EXPORTED_FROM_ABI size_t __next_prime(size_t __n);
+_LIBCPP_HIDE_FROM_ABI inline void __check_for_overflow(size_t __n) {
+ if _LIBCPP_CONSTEXPR (sizeof(size_t) == 4) {
+ if (__n > 0xFFFFFFFB)
+ std::__throw_overflow_error("__next_prime overflow");
+ } else {
+ if (__n > 0xFFFFFFFFFFFFFFC5ull)
+ std::__throw_overflow_error("__next_prime overflow");
+ }
+}
+
+#if _LIBCPP_AVAILABILITY_HAS_NEXT_PRIME_IMPL
+[[__gnu__::__const__]] _LIBCPP_EXPORTED_FROM_ABI size_t __next_prime_impl(size_t) _NOEXCEPT;
+
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline size_t __get_next_prime(size_t __n) {
+ __check_for_overflow(__n);
+ return __next_prime_impl(__n);
+}
+#else
+_LIBCPP_EXPORTED_FROM_ABI size_t __next_prime(size_t);
+
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline size_t __get_next_prime(size_t __n) { return __next_prime(__n); }
+#endif
template <class _NodePtr>
struct __hash_node_base {
@@ -1764,7 +1786,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__rehash(size_type __n) _LIBCPP_D
if (__n == 1)
__n = 2;
else if (__n & (__n - 1))
- __n = std::__next_prime(__n);
+ __n = std::__get_next_prime(__n);
size_type __bc = bucket_count();
if (__n > __bc)
__do_rehash<_UniqueKeys>(__n);
@@ -1772,7 +1794,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__rehash(size_type __n) _LIBCPP_D
__n = std::max<size_type>(
__n,
std::__is_hash_power2(__bc) ? std::__next_hash_pow2(size_t(__math::ceil(float(size()) / max_load_factor())))
- : std::__next_prime(size_t(__math::ceil(float(size()) / max_load_factor()))));
+ : std::__get_next_prime(size_t(__math::ceil(float(size()) / max_load_factor()))));
if (__n < __bc)
__do_rehash<_UniqueKeys>(__n);
}
diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
index 8c55c4385f6f6..4dc197119d7da 100644
--- a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1147,6 +1147,7 @@
{'is_defined': True, 'name': '_ZNSt3__117__assoc_sub_state4waitEv', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__117__assoc_sub_state9__executeEv', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__117__assoc_sub_state9set_valueEv', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__117__next_prime_implEm', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__117__widen_from_utf8ILm16EED0Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__117__widen_from_utf8ILm16EED1Ev', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__117__widen_from_utf8ILm16EED2Ev', 'type': 'FUNC'}
diff --git a/libcxx/src/hash.cpp b/libcxx/src/hash.cpp
index e1e6d2b4c2bdb..76a2b7085d38b 100644
--- a/libcxx/src/hash.cpp
+++ b/libcxx/src/hash.cpp
@@ -51,17 +51,7 @@ const unsigned indices[] = {
// are fewer potential primes to search, and fewer potential primes to divide
// against.
-inline void __check_for_overflow(size_t N) {
- if constexpr (sizeof(size_t) == 4) {
- if (N > 0xFFFFFFFB)
- std::__throw_overflow_error("__next_prime overflow");
- } else {
- if (N > 0xFFFFFFFFFFFFFFC5ull)
- std::__throw_overflow_error("__next_prime overflow");
- }
-}
-
-size_t __next_prime(size_t n) {
+size_t __next_prime_impl(size_t n) noexcept {
const size_t L = 210;
const size_t N = sizeof(small_primes) / sizeof(small_primes[0]);
// If n is small enough, search in small_primes
@@ -446,4 +436,6 @@ size_t __next_prime(size_t n) {
}
}
+_LIBCPP_EXPORTED_FROM_ABI size_t __next_prime(size_t n) { return __get_next_prime(n); }
+
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/test/libcxx/containers/unord/next_prime.pass.cpp b/libcxx/test/libcxx/containers/unord/next_prime.pass.cpp
index c4daabb63e4cf..64d2cb294016e 100644
--- a/libcxx/test/libcxx/containers/unord/next_prime.pass.cpp
+++ b/libcxx/test/libcxx/containers/unord/next_prime.pass.cpp
@@ -12,7 +12,7 @@
// <__hash_table>
-// size_t __next_prime(size_t n);
+// size_t __get_next_prime(size_t n);
// If n == 0, return 0, else return the lowest prime greater than or equal to n
@@ -36,9 +36,9 @@ bool is_prime(std::size_t n) {
}
int main(int, char**) {
- assert(std::__next_prime(0) == 0);
+ assert(std::__get_next_prime(0) == 0);
for (std::size_t n = 1; n <= 100000; ++n) {
- std::size_t p = std::__next_prime(n);
+ std::size_t p = std::__get_next_prime(n);
assert(p >= n);
for (std::size_t i = n; i < p; ++i)
assert(!is_prime(i));
diff --git a/libcxx/test/libcxx/transitive_includes/cxx26.csv b/libcxx/test/libcxx/transitive_includes/cxx26.csv
index 5f906338f4b7c..502ccd5f48b30 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx26.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx26.csv
@@ -1106,6 +1106,7 @@ unordered_set cstring
unordered_set initializer_list
unordered_set limits
unordered_set optional
+unordered_set stdexcept
unordered_set tuple
unordered_set version
utility compare
|
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.
Does this have a noticeable impact on the benchmarks?
} | ||
} | ||
|
||
_LIBCPP_EXPORTED_FROM_ABI size_t __next_prime(size_t n) { return __get_next_prime(n); } |
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.
We should add a comment explaining that this is only provided for backwards compatibility.
// <__hash_table> | ||
|
||
// size_t __next_prime(size_t n); | ||
// size_t __get_next_prime(size_t n); |
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.
Should the name of the test change?
[[__gnu__::__const__]] _LIBCPP_EXPORTED_FROM_ABI size_t __next_prime_impl(size_t) _NOEXCEPT; | ||
|
||
[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI inline size_t __get_next_prime(size_t __n) { | ||
__check_for_overflow(__n); |
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.
I know this isn't necessary for ADL, but let's qualify these calls with std::
for consistency.
} | ||
|
||
#if _LIBCPP_AVAILABILITY_HAS_NEXT_PRIME_IMPL | ||
[[__gnu__::__const__]] _LIBCPP_EXPORTED_FROM_ABI size_t __next_prime_impl(size_t) _NOEXCEPT; |
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.
I feel like a short comment explaining what this does and why there's also __next_prime
would be helpful. This seems confusing for someone not familiar with this patch. Maybe just explain that a new __next_prime_impl
was introduce to allow inlining the __check_overflow
call and this dance deals with backwards compatibility.
This can improve the codegen a bit, since the compiler can now know that
__next_prime
never modifies memory. If the argument to__rehash
is known it can even eliminate the overflow check entirely, possibly allowing the compiler to fold away more code.