-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Use _BitInt and __builtin_popcountg in bitset::count() #160679
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
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis has multiple benefits:
Currently this applies only to Full diff: https://github.com/llvm/llvm-project/pull/160679.diff 1 Files Affected:
diff --git a/libcxx/include/bitset b/libcxx/include/bitset
index e2b46154ae730..d8bb938456104 100644
--- a/libcxx/include/bitset
+++ b/libcxx/include/bitset
@@ -867,7 +867,14 @@ bitset<_Size>::to_string(char __zero, char __one) const {
template <size_t _Size>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 size_t bitset<_Size>::count() const _NOEXCEPT {
- return static_cast<size_t>(std::count(__base::__make_iter(0), __base::__make_iter(_Size), true));
+#ifdef _LIBCPP_COMPILER_CLANG_BASED
+ if constexpr (_Size <= __base::__bits_per_word) {
+ return __builtin_popcountg(static_cast<unsigned _BitInt(_Size)>(__base::__first_));
+ } else
+#endif
+ {
+ return static_cast<size_t>(std::count(__base::__make_iter(0), __base::__make_iter(_Size), true));
+ }
}
template <size_t _Size>
|
28df28b
to
d820b2c
Compare
return 0; | ||
} else if constexpr (_Size <= __base::__bits_per_word) { | ||
return __builtin_popcountg(static_cast<unsigned _BitInt(_Size)>(__base::__first_)); | ||
} else |
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.
Do we really need this else
? Can't we have
#if defined(CLANG)
if constexpr (...) {
// A
} else if constexpr (...) {
// B
}
#endif
return std::count(...);
Seems a bit simpler and equivalent. Are you worried about the mere presence of return std::count(...)
preventing inlining?
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 don't need the else
, but it improves compile times quite a bit, since we avoid instantiating std::count
altogether with it. IMO the tiny simplification isn't worth the compile time increase.
d820b2c
to
c5c359b
Compare
…#160679) This has multiple benefits: 1) The compiler has to do way less work to figure out things fold into a simple `popcount`, improving compile times quite a bit 2) The compiler inlines better, since the compile doesn't have to do complicated optimizations to get to the same point. Looking at the pipeline, it seems that without this, LLVM has to go all the way to GVN to get to the same code as there is after the first InstCombine pass with this change. Currently this applies only to `bitset`s with at most 64 bits, but that is by far the most common case.
This has multiple benefits:
popcount
, improving compile times quite a bitCurrently this applies only to
bitset
s with at most 64 bits, but that is by far the most common case.