-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[libc++] Decrease instantiation cost of __constexpr_memmove #125109
Conversation
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesUsing Full diff: https://github.com/llvm/llvm-project/pull/125109.diff 1 Files Affected:
diff --git a/libcxx/include/__string/constexpr_c_functions.h b/libcxx/include/__string/constexpr_c_functions.h
index 0bc128b68b5799..fbe7e10d440ce1 100644
--- a/libcxx/include/__string/constexpr_c_functions.h
+++ b/libcxx/include/__string/constexpr_c_functions.h
@@ -204,23 +204,26 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp& __assign_trivially_copy
return __dest;
}
-template <class _Tp, class _Up, __enable_if_t<__is_always_bitcastable<_Up, _Tp>::value, int> = 0>
+template <class _Tp, class _Up>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp*
__constexpr_memmove(_Tp* __dest, _Up* __src, __element_count __n) {
+ static_assert(__is_always_bitcastable<_Up, _Tp>::value);
size_t __count = static_cast<size_t>(__n);
if (__libcpp_is_constant_evaluated()) {
#ifdef _LIBCPP_COMPILER_CLANG_BASED
- if (is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value) {
+ if _LIBCPP_CONSTEXPR (is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value) {
::__builtin_memmove(__dest, __src, __count * sizeof(_Tp));
return __dest;
- }
+ } else
#endif
- if (std::__is_pointer_in_range(__src, __src + __count, __dest)) {
- for (; __count > 0; --__count)
- std::__assign_trivially_copyable(__dest[__count - 1], __src[__count - 1]);
- } else {
- for (size_t __i = 0; __i != __count; ++__i)
- std::__assign_trivially_copyable(__dest[__i], __src[__i]);
+ {
+ if (std::__is_pointer_in_range(__src, __src + __count, __dest)) {
+ for (; __count > 0; --__count)
+ std::__assign_trivially_copyable(__dest[__count - 1], __src[__count - 1]);
+ } else {
+ for (size_t __i = 0; __i != __count; ++__i)
+ std::__assign_trivially_copyable(__dest[__i], __src[__i]);
+ }
}
} else if (__count > 0) {
::__builtin_memmove(__dest, __src, (__count - 1) * sizeof(_Tp) + __datasizeof_v<_Tp>);
|
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.
Thanks, LGTM!
Just curious, do you have any numbers on the compilation performance improvement?
FYI I fixed a minor typo in never
in the commit message.
IIRC for every instantiation where the condition is true you save ~0.8ms. Not huge on its own, but given that it's instantiated quite a bit it's probably a nice improvement across an entire code base.
Thanks! |
) Using `if constexpr` in `__constexpr_memmove` makes the instantiation three times faster for the same type, since it avoids a bunch of class instantiations and SFINAE for constexpr support that's never actually used. Given that `__constexpr_memmove` is used quite a bit through `std::copy` and is instantiated multiple times when just including `<__string/char_traits.h>` this can provide a nice compile time speedup for a very simple change.
Using
if constexpr
in__constexpr_memmove
makes the instantiation three times faster for the same type, since it avoids a bunch of class instantiations and SFINAE for constexpr support that's never actually used. Given that__constexpr_memmove
is used quite a bit throughstd::copy
and is instantiated multiple times when just including<__string/char_traits.h>
this can provide a nice compile time speedup for a very simple change.