Skip to content

Conversation

@PaulXiCao
Copy link
Contributor

Closes #131179.

@PaulXiCao PaulXiCao requested a review from a team as a code owner March 23, 2025 22:07
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 23, 2025

@llvm/pr-subscribers-libcxx

Author: None (PaulXiCao)

Changes

Closes #131179.


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

3 Files Affected:

  • (modified) libcxx/include/__algorithm/sort.h (+2-2)
  • (modified) libcxx/include/__cxx03/__algorithm/find.h (+3-3)
  • (modified) libcxx/include/__cxx03/__algorithm/sort.h (+2-2)
diff --git a/libcxx/include/__algorithm/sort.h b/libcxx/include/__algorithm/sort.h
index 8dd0721f2c65f..015d6531b1d87 100644
--- a/libcxx/include/__algorithm/sort.h
+++ b/libcxx/include/__algorithm/sort.h
@@ -359,9 +359,9 @@ inline _LIBCPP_HIDE_FROM_ABI void __swap_bitmap_pos(
   // Swap one pair on each iteration as long as both bitsets have at least one
   // element for swapping.
   while (__left_bitset != 0 && __right_bitset != 0) {
-    difference_type __tz_left  = __libcpp_ctz(__left_bitset);
+    difference_type __tz_left  = __countr_zero(__left_bitset);
     __left_bitset              = __libcpp_blsr(__left_bitset);
-    difference_type __tz_right = __libcpp_ctz(__right_bitset);
+    difference_type __tz_right = __countr_zero(__right_bitset);
     __right_bitset             = __libcpp_blsr(__right_bitset);
     _Ops::iter_swap(__first + __tz_left, __last - __tz_right);
   }
diff --git a/libcxx/include/__cxx03/__algorithm/find.h b/libcxx/include/__cxx03/__algorithm/find.h
index ff5ac9b8b1bd0..29486e9438711 100644
--- a/libcxx/include/__cxx03/__algorithm/find.h
+++ b/libcxx/include/__cxx03/__algorithm/find.h
@@ -108,7 +108,7 @@ __find_bool(__bit_iterator<_Cp, _IsConst> __first, typename _Cp::size_type __n)
     __storage_type __m     = (~__storage_type(0) << __first.__ctz_) & (~__storage_type(0) >> (__clz_f - __dn));
     __storage_type __b     = std::__invert_if<!_ToFind>(*__first.__seg_) & __m;
     if (__b)
-      return _It(__first.__seg_, static_cast<unsigned>(std::__libcpp_ctz(__b)));
+      return _It(__first.__seg_, static_cast<unsigned>(std::__countr_zero(__b)));
     if (__n == __dn)
       return __first + __n;
     __n -= __dn;
@@ -118,14 +118,14 @@ __find_bool(__bit_iterator<_Cp, _IsConst> __first, typename _Cp::size_type __n)
   for (; __n >= __bits_per_word; ++__first.__seg_, __n -= __bits_per_word) {
     __storage_type __b = std::__invert_if<!_ToFind>(*__first.__seg_);
     if (__b)
-      return _It(__first.__seg_, static_cast<unsigned>(std::__libcpp_ctz(__b)));
+      return _It(__first.__seg_, static_cast<unsigned>(std::__countr_zero(__b)));
   }
   // do last partial word
   if (__n > 0) {
     __storage_type __m = ~__storage_type(0) >> (__bits_per_word - __n);
     __storage_type __b = std::__invert_if<!_ToFind>(*__first.__seg_) & __m;
     if (__b)
-      return _It(__first.__seg_, static_cast<unsigned>(std::__libcpp_ctz(__b)));
+      return _It(__first.__seg_, static_cast<unsigned>(std::__countr_zero(__b)));
   }
   return _It(__first.__seg_, static_cast<unsigned>(__n));
 }
diff --git a/libcxx/include/__cxx03/__algorithm/sort.h b/libcxx/include/__cxx03/__algorithm/sort.h
index 009ebf717bbd8..47472afbbe562 100644
--- a/libcxx/include/__cxx03/__algorithm/sort.h
+++ b/libcxx/include/__cxx03/__algorithm/sort.h
@@ -399,9 +399,9 @@ inline _LIBCPP_HIDE_FROM_ABI void __swap_bitmap_pos(
   // Swap one pair on each iteration as long as both bitsets have at least one
   // element for swapping.
   while (__left_bitset != 0 && __right_bitset != 0) {
-    difference_type __tz_left  = __libcpp_ctz(__left_bitset);
+    difference_type __tz_left  = __countr_zero(__left_bitset);
     __left_bitset              = __libcpp_blsr(__left_bitset);
-    difference_type __tz_right = __libcpp_ctz(__right_bitset);
+    difference_type __tz_right = __countr_zero(__right_bitset);
     __right_bitset             = __libcpp_blsr(__right_bitset);
     _Ops::iter_swap(__first + __tz_left, __last - __tz_right);
   }

@winner245
Copy link
Contributor

Hi @PaulXiCao, thanks for your efforts in addressing this issue! Just to provide some context—this issue was opened a week ago as a follow-up to my previous work and was assigned to me shortly thereafter. I’ve been actively working on related patches, but since this change depends on dropping Clang 18 support, I hadn’t submitted a patch yet.

While I truly appreciate your enthusiasm and contribution, I think it would be helpful in the future to check with the assignee before working on a recently assigned issue, especially if it’s a follow-up to their prior work. This can help avoid duplicated effort and ensure smoother collaboration. Thanks again!

@mordante
Copy link
Member

Hi @PaulXiCao, thanks for your efforts in addressing this issue! Just to provide some context—this issue was opened a week ago as a follow-up to my previous work and was assigned to me shortly thereafter. I’ve been actively working on related patches, but since this change depends on dropping Clang 18 support, I hadn’t submitted a patch yet.

While I truly appreciate your enthusiasm and contribution, I think it would be helpful in the future to check with the assignee before working on a recently assigned issue, especially if it’s a follow-up to their prior work. This can help avoid duplicated effort and ensure smoother collaboration. Thanks again!

FYI @winner245 removing Clang-18 support is blocked by some CI runners, the maintainers for these runners are working on upgrading the clang version of their runners.

@winner245
Copy link
Contributor

Hi @mordante, thank you for the update on the current status of removing Clang 18 support, which is currently blocked by some CI runners. I understand that the changes in this patch will need to be put on hold until Clang 18 support is officially dropped (@PaulXiCao).

@mordante
Copy link
Member

Hi @mordante, thank you for the update on the current status of removing Clang 18 support, which is currently blocked by some CI runners. I understand that the changes in this patch will need to be put on hold until Clang 18 support is officially dropped (@PaulXiCao).

Looking at the status of the CI this works in Clang-18; the ARM runners are still on Clang-18.

@winner245
Copy link
Contributor

Hi @mordante, thank you for the update on the current status of removing Clang 18 support, which is currently blocked by some CI runners. I understand that the changes in this patch will need to be put on hold until Clang 18 support is officially dropped (@PaulXiCao).

Looking at the status of the CI this works in Clang-18; the ARM runners are still on Clang-18.

The primary goal of #131179 (based on my discussion with @ldionne in #122641 (comment) ) was to eliminate all usages of __libcpp_ctz in favor of the built-in implementation available since Clang 19. Ultimately, this allows us to remove functions like __libcpp_ctz, but only when we officially drop support for Clang 18.

That being said, the current patch doesn't fully address issue #131179. Most of the changes are applied to frozen header files, which I believe should stay unchanged unless there is a strong reason. That's why I didn't make these changes in my earlier patch.

@mordante
Copy link
Member

Hi @mordante, thank you for the update on the current status of removing Clang 18 support, which is currently blocked by some CI runners. I understand that the changes in this patch will need to be put on hold until Clang 18 support is officially dropped (@PaulXiCao).

Looking at the status of the CI this works in Clang-18; the ARM runners are still on Clang-18.

The primary goal of #131179 (based on my discussion with @ldionne in #122641 (comment) ) was to eliminate all usages of __libcpp_ctz in favor of the built-in implementation available since Clang 19. Ultimately, this allows us to remove functions like __libcpp_ctz, but only when we officially drop support for Clang 18.

I didn't look into the patch or which Clang version supports the built-in. If you say that's Clang-19 this patch indeed needs to be post-poned until we can drop Clang-18 support. I mainly wanted to update you on the status of Clang-18 support.

@ldionne
Copy link
Member

ldionne commented Mar 25, 2025

Closing per @winner245 's comment -- we already have a patch ready to go once Clang 18 is dropped, and this patch as-is doesn't implement all that we wanted from #131179.

@ldionne ldionne closed this Mar 25, 2025
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.

[libc++] Replace uses of __libcpp_ctz by __countr_zero

5 participants