[libcxx-commits] [PATCH] D144394: [libc++] Forward to std::{, w}memcmp in std::find

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/find.h:54
+          __enable_if_t<__is_identity<_Proj>::value && __is_trivially_equality_comparable<_Tp, _Up>::value &&
+                            sizeof(_Tp) == sizeof(wchar_t) && alignof(_Tp) >= alignof(wchar_t),
+                        int> = 0>
----------------
Mordante wrote:
> philnik wrote:
> > Mordante wrote:
> > > Why not test `is_same_v<_Tp, wchar_t>`?
> > Because that would disable the optimization for other types that have the same size as `wchar_t`. For these types this optimization is probably UB, but that shouldn't be a problem in practice, since it's on an ABI boundary and no compiler currently optimizes `wmemchr` specifically (and IMO no compiler should ever optimize based on this, since there is no potential gain I can see).
> The function you call has the following signature
> 
> ```
> inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 const wchar_t*
> __constexpr_wmemchr(const wchar_t* __str, wchar_t __char, size_t __count)
> ```
> So how can this work when `_Tp` is not a `wchar_t`? That requires a `reinterpret_cast` which is not allowed during constant evaluation. What am I missing?
> 
Your signature is wrong, `__constexpr_wmemchr` is templated.



================
Comment at: libcxx/include/__string/constexpr_c_functions.h:73-83
+#if _LIBCPP_STD_VER >= 17 && __has_builtin(__builtin_char_memchr)
+    if constexpr (is_same<_Tp, char>::value)
+      return __builtin_char_memchr(__str, __char, __count);
 #endif
+
+    for (; __count; --__count) {
+      if (*__str == __char)
----------------
Mordante wrote:
> philnik wrote:
> > Mordante wrote:
> > > Note that the current way it's written generates unneeded code
> > This looks quite surprising to me, and I'm not sure it's worth the little bit of time the compiler spends on this.
> This is something we have used before. Now it will generate dead code.
clang doesn't even emit the condition from the fronted AFAICT, let alone the backend: https://godbolt.org/z/87bvzbhbT. This is at best a minimal compile time speedup.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:78
+  types::for_each(types::integer_types(), TestTypes<long long>());
+  types::for_each(types::integer_types(), TestTypes<Comparable>());
+
----------------
Mordante wrote:
> I really would like to see tests for all types that will use this optimization. Especially to make sure the code is valid during constant evaluation.
We currently only have integral types and pointers marked trivially equality comparable,  and I don't think it's worth testing for pointers specifically, since they are for all intents and purposes integers with a funny syntax in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144394/new/

https://reviews.llvm.org/D144394