Skip to content

[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

Merged
merged 3 commits into from
Mar 26, 2025

Conversation

winner245
Copy link
Contributor

@winner245 winner245 commented Dec 30, 2024

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 public bitset). So __pos / __bits_per_word == 0 and __pos / __pos % __bits_per_word == __pos.

@winner245 winner245 force-pushed the remove-division-modulo branch from 4c4ebac to acff5cf Compare February 12, 2025 16:26
@winner245 winner245 marked this pull request as ready for review March 18, 2025 15:59
@winner245 winner245 requested a review from a team as a code owner March 18, 2025 15:59
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 18, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2025

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The PR removes the unnecessary division and modulo operations in the one-word specialization __bitset&lt;1, _Size&gt;. The reason is that for the one-word specialization, we have __pos &lt; __bits_per_word (as __bitset&lt;1, _Size&gt; is an implementation detail only used by the public bitset). So __pos / __bits_per_word == 0 and __pos / __pos % __bits_per_word == __pos.


Full diff: https://github.com/llvm/llvm-project/pull/121312.diff

1 Files Affected:

  • (modified) libcxx/include/bitset (+2-2)
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;

Copy link
Member

@ldionne ldionne left a 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!

@winner245 winner245 force-pushed the remove-division-modulo branch 2 times, most recently from 7a4133f to 0f76a81 Compare March 19, 2025 22:47
@winner245 winner245 force-pushed the remove-division-modulo branch from 0f76a81 to 9f8c637 Compare March 26, 2025 02:15
@@ -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 {
Copy link
Member

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

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __bit_iterator& operator+=(difference_type __n) {
, which might not produce efficient code. I still think we should investigate that since that looks like the way we'd want to write our code at a high level.

Copy link
Member

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

@ldionne ldionne merged commit 649cbcc into llvm:main Mar 26, 2025
81 checks passed
@winner245 winner245 deleted the remove-division-modulo branch March 26, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants