-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libc++] Remove unnecessary division and modulo operations in bitset #121312
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
4c4ebac
to
acff5cf
Compare
@llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesThe PR removes the unnecessary division and modulo operations in the one-word specialization Full diff: https://github.com/llvm/llvm-project/pull/121312.diff 1 Files Affected:
diff --git a/libcxx/include/bitset b/libcxx/include/bitset
index a20842985b3d5..31e960cc8797b 100644
--- a/libcxx/include/bitset
+++ b/libcxx/include/bitset
@@ -465,10 +465,10 @@ protected:
return __const_reference(&__first_, __storage_type(1) << __pos);
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __iterator __make_iter(size_t __pos) _NOEXCEPT {
- return __iterator(&__first_ + __pos / __bits_per_word, __pos % __bits_per_word);
+ return __iterator(&__first_, __pos);
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __const_iterator __make_iter(size_t __pos) const _NOEXCEPT {
- return __const_iterator(&__first_ + __pos / __bits_per_word, __pos % __bits_per_word);
+ return __const_iterator(&__first_, __pos);
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 void operator&=(const __bitset& __v) _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.
LGTM with suggestions applied, thanks for noticing this!
7a4133f
to
0f76a81
Compare
0f76a81
to
9f8c637
Compare
@@ -464,10 +464,14 @@ protected: | |||
return __const_reference(&__first_, __storage_type(1) << __pos); | |||
} | |||
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __iterator __make_iter(size_t __pos) _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 am tempted to see whether we can introduce __begin()
and __end()
functions instead, and get rid of __make_iter
entirely. I'd imagine something like
iterator __begin() { return /* __make_iter(0); */ }
iterator __end() { return /* __make_iter(_Size); */ }
// and then here's an example usage
// OLD:
std::copy_backward(__base::__make_iter(0), __base::__make_iter(_Size - __pos), __base::__make_iter(_Size));
// NEW:
std::copy_backward(__begin(), __end() - __pos, __end());
Now the main question IMO is whether __end() - __pos
produces equivalent code. I think we'll end up calling
llvm-project/libcxx/include/__bit_reference
Line 368 in 52f941a
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __bit_iterator& operator+=(difference_type __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 filed that as #133111, let's do it in a follow-up
The PR removes the unnecessary division and modulo operations in the one-word specialization
__bitset<1, _Size>
. The reason is that for the one-word specialization, we have__pos < __bits_per_word
(as__bitset<1, _Size>
is an implementation detail only used by the publicbitset
). So__pos / __bits_per_word == 0
and__pos / __pos % __bits_per_word == __pos
.