Skip to content

[libc++] Remove a few unnecessary branches from basic_string::find #137266

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 1 commit into from
May 7, 2025

Conversation

philnik777
Copy link
Contributor

I've recently looked at the assembly for basic_string::find() and realized that there were a few branches I didn't expect. It turns out that we check for null before calling __constexpr_memchr in some cases, which the compiler doesn't optimize away. This is a really uncommon case though, so I'm not convinced it makes a ton of sense to optimize for that.
The second case is where __pos >= __sz. There, we can instead check __pos > __sz, which the optimizer is able to remove if __pos == 0, which is also a quite common case (basic_string::find(CharT), without an explicit size parameter).

@philnik777 philnik777 marked this pull request as ready for review April 25, 2025 08:40
@philnik777 philnik777 requested a review from a team as a code owner April 25, 2025 08:40
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 25, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

I've recently looked at the assembly for basic_string::find() and realized that there were a few branches I didn't expect. It turns out that we check for null before calling __constexpr_memchr in some cases, which the compiler doesn't optimize away. This is a really uncommon case though, so I'm not convinced it makes a ton of sense to optimize for that.
The second case is where __pos >= __sz. There, we can instead check __pos > __sz, which the optimizer is able to remove if __pos == 0, which is also a quite common case (basic_string::find(CharT), without an explicit size parameter).


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

1 Files Affected:

  • (modified) libcxx/include/__string/char_traits.h (+1-5)
diff --git a/libcxx/include/__string/char_traits.h b/libcxx/include/__string/char_traits.h
index 60ec186ead826..86c92477cbfeb 100644
--- a/libcxx/include/__string/char_traits.h
+++ b/libcxx/include/__string/char_traits.h
@@ -132,8 +132,6 @@ struct char_traits<char> {
 
   static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 const char_type*
   find(const char_type* __s, size_t __n, const char_type& __a) _NOEXCEPT {
-    if (__n == 0)
-      return nullptr;
     return std::__constexpr_memchr(__s, __a, __n);
   }
 
@@ -250,8 +248,6 @@ struct char_traits<wchar_t> : __char_traits_base<wchar_t, wint_t, static_cast<wi
 
   static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 const char_type*
   find(const char_type* __s, size_t __n, const char_type& __a) _NOEXCEPT {
-    if (__n == 0)
-      return nullptr;
     return std::__constexpr_wmemchr(__s, __a, __n);
   }
 };
@@ -352,7 +348,7 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX17 size_t char_traits<char32_t>::length(const
 template <class _CharT, class _SizeT, class _Traits, _SizeT __npos>
 inline _SizeT _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI
 __str_find(const _CharT* __p, _SizeT __sz, _CharT __c, _SizeT __pos) _NOEXCEPT {
-  if (__pos >= __sz)
+  if (__pos > __sz)
     return __npos;
   const _CharT* __r = _Traits::find(__p + __pos, __sz - __pos, __c);
   if (__r == nullptr)

@ldionne ldionne merged commit 38595fb into llvm:main May 7, 2025
84 checks passed
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