Skip to content

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