Skip to content

Conversation

nickdesaulniers
Copy link
Member

libc/src/__support/CPP/bit.h and cpp:: is meant to mirror std::. Fix the TODO.

libc/src/__support/CPP/bit.h and cpp:: is meant to mirror std::. Fix the TODO.
@llvmbot llvmbot added the libc label Mar 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 7, 2024

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

libc/src/__support/CPP/bit.h and cpp:: is meant to mirror std::. Fix the TODO.


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

6 Files Affected:

  • (modified) libc/src/__support/CPP/bit.h (+4-6)
  • (modified) libc/src/stdbit/stdc_count_ones_uc.cpp (+1-1)
  • (modified) libc/src/stdbit/stdc_count_ones_ui.cpp (+1-1)
  • (modified) libc/src/stdbit/stdc_count_ones_ul.cpp (+1-1)
  • (modified) libc/src/stdbit/stdc_count_ones_ull.cpp (+1-1)
  • (modified) libc/src/stdbit/stdc_count_ones_us.cpp (+1-1)
diff --git a/libc/src/__support/CPP/bit.h b/libc/src/__support/CPP/bit.h
index 6b625b0c97a365..015a55cc53a214 100644
--- a/libc/src/__support/CPP/bit.h
+++ b/libc/src/__support/CPP/bit.h
@@ -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) {
   int count = 0;
   for (int i = 0; i != cpp::numeric_limits<T>::digits; ++i)
     if ((value >> i) & 0x1)
@@ -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)
@@ -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
diff --git a/libc/src/stdbit/stdc_count_ones_uc.cpp b/libc/src/stdbit/stdc_count_ones_uc.cpp
index 5a7314caa3baa0..1e998ff521b7db 100644
--- a/libc/src/stdbit/stdc_count_ones_uc.cpp
+++ b/libc/src/stdbit/stdc_count_ones_uc.cpp
@@ -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
diff --git a/libc/src/stdbit/stdc_count_ones_ui.cpp b/libc/src/stdbit/stdc_count_ones_ui.cpp
index 289f4bac31f7b8..e457dd793db33d 100644
--- a/libc/src/stdbit/stdc_count_ones_ui.cpp
+++ b/libc/src/stdbit/stdc_count_ones_ui.cpp
@@ -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
diff --git a/libc/src/stdbit/stdc_count_ones_ul.cpp b/libc/src/stdbit/stdc_count_ones_ul.cpp
index 83f3279d791937..ed86653fc7ee2e 100644
--- a/libc/src/stdbit/stdc_count_ones_ul.cpp
+++ b/libc/src/stdbit/stdc_count_ones_ul.cpp
@@ -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
diff --git a/libc/src/stdbit/stdc_count_ones_ull.cpp b/libc/src/stdbit/stdc_count_ones_ull.cpp
index 104788aaf21265..c5ecc3cda6477a 100644
--- a/libc/src/stdbit/stdc_count_ones_ull.cpp
+++ b/libc/src/stdbit/stdc_count_ones_ull.cpp
@@ -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
diff --git a/libc/src/stdbit/stdc_count_ones_us.cpp b/libc/src/stdbit/stdc_count_ones_us.cpp
index 4b6ff0b94b626a..465c5c374e7c64 100644
--- a/libc/src/stdbit/stdc_count_ones_us.cpp
+++ b/libc/src/stdbit/stdc_count_ones_us.cpp
@@ -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

Copy link

github-actions bot commented Mar 7, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

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.

@jhuber6
Copy link
Contributor

jhuber6 commented Mar 7, 2024

Seems you missed one uses in the tests.

@nickdesaulniers
Copy link
Member Author

Seems you missed one uses in the tests.

Fixed in 36bfffb PTAL

@nickdesaulniers nickdesaulniers merged commit 293ec48 into llvm:main Mar 7, 2024
@nickdesaulniers nickdesaulniers deleted the popcount branch March 7, 2024 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants