-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Add constant folding for optimized std::find variants #96491
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
Conversation
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesCurrently Clang isn't able to constant fold While not necessarily representative of the real world, an optimized version of the Full diff: https://github.com/llvm/llvm-project/pull/96491.diff 2 Files Affected:
diff --git a/libcxx/include/__string/constexpr_c_functions.h b/libcxx/include/__string/constexpr_c_functions.h
index a978f816f1897..cd5b63ae47d1e 100644
--- a/libcxx/include/__string/constexpr_c_functions.h
+++ b/libcxx/include/__string/constexpr_c_functions.h
@@ -132,6 +132,19 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp* __constexpr_memchr(_Tp*
static_assert(sizeof(_Tp) == 1 && __libcpp_is_trivially_equality_comparable<_Tp, _Up>::value,
"Calling memchr on non-trivially equality comparable types is unsafe.");
+ if (__builtin_constant_p(__count) && __builtin_constant_p(__value)) {
+ for (; __count; --__count) {
+ if (!__builtin_constant_p(*__str))
+ break;
+ if (*__str == __value)
+ return __str;
+ ++__str;
+ }
+ }
+
+ if (__builtin_constant_p(__count) && __count == 0)
+ return nullptr;
+
if (__libcpp_is_constant_evaluated()) {
// use __builtin_char_memchr to optimize constexpr evaluation if we can
#if _LIBCPP_STD_VER >= 17 && __has_builtin(__builtin_char_memchr)
diff --git a/libcxx/include/cwchar b/libcxx/include/cwchar
index 08cfac58c846a..76fdf08988dc5 100644
--- a/libcxx/include/cwchar
+++ b/libcxx/include/cwchar
@@ -231,6 +231,19 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp* __constexpr_wmemchr(_Tp
__libcpp_is_trivially_equality_comparable<_Tp, _Tp>::value,
"Calling wmemchr on non-trivially equality comparable types is unsafe.");
+ if (__builtin_constant_p(__count) && __builtin_constant_p(__value)) {
+ for (; __count; --__count) {
+ if (!__builtin_constant_p(*__str))
+ break;
+ if (*__str == __value)
+ return __str;
+ ++__str;
+ }
+ }
+
+ if (__builtin_constant_p(__count) && __count == 0)
+ return nullptr;
+
#if __has_builtin(__builtin_wmemchr)
if (!__libcpp_is_constant_evaluated()) {
wchar_t __value_buffer = 0;
|
| if (__builtin_constant_p(__count) && __count == 0) | ||
| return nullptr; | ||
|
|
||
| if (__libcpp_is_constant_evaluated()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of this branch since __builtin_constant_p is always true inside a constant expression. IOW, the code path above is always taken when __libcpp_is_constant_evaluated() is true, so the branch below is basically dead now.
| static_assert(sizeof(_Tp) == 1 && __libcpp_is_trivially_equality_comparable<_Tp, _Up>::value, | ||
| "Calling memchr on non-trivially equality comparable types is unsafe."); | ||
|
|
||
| if (__builtin_constant_p(__count) && __builtin_constant_p(__value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some kind of comment explaining that this is to aid constant-folding and it also handles evaluation inside constant expressions, since that is not super obvious.
|
|
||
| if (__libcpp_is_constant_evaluated()) { | ||
| // use __builtin_char_memchr to optimize constexpr evaluation if we can | ||
| #if _LIBCPP_STD_VER >= 17 && __has_builtin(__builtin_char_memchr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we file a bug report against Clang to complain that __builtin_memchr doesn't constant-fold? It seems like an obvious QOI issue since Clang can already constant-evaluate that function.
ldionne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From live review: It seems like Clang only fails to constant fold this for a local array. Since that's the case, IMO it would make sense to file a bug against the optimizer instead of working around it in the library.
Currently Clang isn't able to constant fold
memchrandwmemchr. This adds some basic constant folding to__constexpr_memchrand__constexpr_wmemchr, mostly alleviating the problem.While not necessarily representative of the real world, an optimized version of the
find.pass.cpptest is reduced from ~13k to ~4.5k lines of assembly.