[libcxx-commits] [PATCH] D139554: [libc++] Forward to std::memcmp for trivially comparable types in equal

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D139554#3986647 <https://reviews.llvm.org/D139554#3986647>, @philnik wrote:

>   --------------------------------------------------------
>   Benchmark                              old           new
>   --------------------------------------------------------
>   bm_equal_iter/1                   0.926 ns       2.08 ns
>   bm_equal_iter/2                    1.36 ns       2.20 ns
>   bm_equal_iter/3                    1.78 ns       2.10 ns

I assume these are slower due to the extra function call overhead.

> bm_equal_iter/32768               15152 ns        485 ns
> bm_equal_iter/262144             120923 ns       4331 ns
> bm_equal_iter/1048576            484883 ns      20014 ns

nice improvements!

> Note that this decreases binary size at the same time, since this forwards to a C library function now.

For my curiosity can you share these numbers?

I'm in general happy with the patch, but I've some concern regarding one of the new type traits. I think it would be good to have a second pair of eyes for ranges experts.



================
Comment at: libcxx/benchmarks/CMakeLists.txt:111
 target_link_options(   cxx-benchmarks-flags-libcxx INTERFACE -nodefaultlibs "-L${BENCHMARK_LIBCXX_INSTALL}/lib" ${SANITIZER_FLAGS})
+target_link_options(   cxx-benchmarks-flags-libcxx INTERFACE -nodefaultlibs "-L${BENCHMARK_LIBCXX_INSTALL}/lib64" ${SANITIZER_FLAGS})
 
----------------
Why is this needed? Is it portable on all (Linux) platforms?


================
Comment at: libcxx/benchmarks/algorithms/equal.bench.cpp:1
+#include <algorithm>
+#include <benchmark/benchmark.h>
----------------
Please add the copyright header.


================
Comment at: libcxx/include/__algorithm/comp.h:14
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_predicate_kind.h>
 
----------------
Can you add these to the `type_traits` header too.


================
Comment at: libcxx/include/__algorithm/ranges_equal.h:24
 #include <__utility/move.h>
+#include <__utility/pair.h>
 
----------------
Was pair already required?
Or is this to fix the modular build?
When it's the latter, the proper fix would be to export this header from `__algorithm/unwrap_range.h`.


================
Comment at: libcxx/include/__type_traits/is_comparable.h:37
+                        __is_equality_comparable<_Tp, _Up>::value && is_integral<_Tp>::value &&
+                            is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value> {};
+
----------------
This looks really odd, and it would be nice to have comment why this design is chosen. 
Why are the following types not in the set, floating-point values, enums, pointers.

Is `__is_trivially_equality_comparable<int, float>` true? The other way around isn't.

I really want some internal tests for this type trait.


================
Comment at: libcxx/test/std/algorithms/robust_against_adl.compile.pass.cpp:14
+// godbolt (https://godbolt.org/z/rsPv8e8fn).
+// UNSUPPORTED: gcc-12
+
----------------
Not entirely happy with this, but lets see what gcc-13 brings in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554