Skip to content

[libc] rename cpp::count_ones to cpp::popcount to better mirror std:: #84388

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.

Sign up for GitHub

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 4 commits into from
Mar 7, 2024
Merged

[libc] rename cpp::count_ones to cpp::popcount to better mirror std:: #84388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64c92d1
[libc] rename cpp::count_ones to cpp::popcount to better mirror std::
nickdesaulniers Mar 7, 2024
bbce9eb
format
nickdesaulniers Mar 7, 2024
36bfffb
fix up bit test
nickdesaulniers Mar 7, 2024
541d22d
another fix
nickdesaulniers Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions libc/src/__support/CPP/bit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,14 +269,12 @@ first_trailing_one(T value) {
return value == cpp::numeric_limits<T>::max() ? 0 : countr_zero(value) + 1;
}

/// Count number of 1's aka population count or hamming weight.
/// Count number of 1's aka population count or Hamming weight.
///
/// Only unsigned integral types are allowed.
// TODO: rename as 'popcount' to follow the standard
// https://en.cppreference.com/w/cpp/numeric/popcount
template <typename T>
[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
count_ones(T value) {
popcount(T value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't use __builtin_popcount? Pretty much every compiler supports it as far as I'm aware, or is there some benefit to relying on compiler optimizations for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://godbolt.org/z/M4sf9rr8Y seems the compiler can't completely figure it out. I'd recommend libc_has_builitin if we can.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't use __builtin_popcount?

128b, but we do use that builtin otherwise. See below (lines 284-295).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't catch it in the context. Someone could probably implement it on __128 in clang at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #82058 which resulted in 21d8332.

We can use __has_builtin to use it for newer compilers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, glad people are on top of things.

int count = 0;
for (int i = 0; i != cpp::numeric_limits<T>::digits; ++i)
if ((value >> i) & 0x1)
Expand All @@ -285,7 +283,7 @@ count_ones(T value) {
}
#define ADD_SPECIALIZATION(TYPE, BUILTIN) \
template <> \
[[nodiscard]] LIBC_INLINE constexpr int count_ones<TYPE>(TYPE value) { \
[[nodiscard]] LIBC_INLINE constexpr int popcount<TYPE>(TYPE value) { \
return BUILTIN(value); \
}
ADD_SPECIALIZATION(unsigned char, __builtin_popcount)
Expand All @@ -300,7 +298,7 @@ ADD_SPECIALIZATION(unsigned long long, __builtin_popcountll)
template <typename T>
[[nodiscard]] LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, int>
count_zeros(T value) {
return count_ones<T>(static_cast<T>(~value));
return popcount<T>(static_cast<T>(~value));
}

} // namespace LIBC_NAMESPACE::cpp
Expand Down
2 changes: 1 addition & 1 deletion libc/src/__support/UInt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ has_single_bit(T value) {
for (auto word : value.val) {
if (word == 0)
continue;
bits += count_ones(word);
bits += popcount(word);
if (bits > 1)
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion libc/src/stdbit/stdc_count_ones_uc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace LIBC_NAMESPACE {

LLVM_LIBC_FUNCTION(unsigned, stdc_count_ones_uc, (unsigned char value)) {
return static_cast<unsigned>(cpp::count_ones(value));
return static_cast<unsigned>(cpp::popcount(value));
}

} // namespace LIBC_NAMESPACE
2 changes: 1 addition & 1 deletion libc/src/stdbit/stdc_count_ones_ui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace LIBC_NAMESPACE {

LLVM_LIBC_FUNCTION(unsigned, stdc_count_ones_ui, (unsigned value)) {
return static_cast<unsigned>(cpp::count_ones(value));
return static_cast<unsigned>(cpp::popcount(value));
}

} // namespace LIBC_NAMESPACE
2 changes: 1 addition & 1 deletion libc/src/stdbit/stdc_count_ones_ul.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace LIBC_NAMESPACE {

LLVM_LIBC_FUNCTION(unsigned, stdc_count_ones_ul, (unsigned long value)) {
return static_cast<unsigned>(cpp::count_ones(value));
return static_cast<unsigned>(cpp::popcount(value));
}

} // namespace LIBC_NAMESPACE
2 changes: 1 addition & 1 deletion libc/src/stdbit/stdc_count_ones_ull.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace LIBC_NAMESPACE {

LLVM_LIBC_FUNCTION(unsigned, stdc_count_ones_ull, (unsigned long long value)) {
return static_cast<unsigned>(cpp::count_ones(value));
return static_cast<unsigned>(cpp::popcount(value));
}

} // namespace LIBC_NAMESPACE
2 changes: 1 addition & 1 deletion libc/src/stdbit/stdc_count_ones_us.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace LIBC_NAMESPACE {

LLVM_LIBC_FUNCTION(unsigned, stdc_count_ones_us, (unsigned short value)) {
return static_cast<unsigned>(cpp::count_ones(value));
return static_cast<unsigned>(cpp::popcount(value));
}

} // namespace LIBC_NAMESPACE
4 changes: 2 additions & 2 deletions libc/test/src/__support/CPP/bit_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,9 @@ TYPED_TEST(LlvmLibcBitTest, CountZeros, UnsignedTypesNoBigInt) {
}

TYPED_TEST(LlvmLibcBitTest, CountOnes, UnsignedTypesNoBigInt) {
EXPECT_EQ(count_ones(T(0)), 0);
EXPECT_EQ(popcount(T(0)), 0);
for (int i = 0; i != cpp::numeric_limits<T>::digits; ++i)
EXPECT_EQ(count_ones<T>(cpp::numeric_limits<T>::max() >> i),
EXPECT_EQ(popcount<T>(cpp::numeric_limits<T>::max() >> i),
cpp::numeric_limits<T>::digits - i);
}

Expand Down