[libcxx-commits] [PATCH] D143596: [libc++] Optmize std::ranges::min for types that are cheap to copy

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think this is an interesting change, thanks for noticing! I have a few comments.



================
Comment at: libcxx/benchmarks/CMakeLists.txt:103
   endif()
 else()
   target_link_libraries(cxx-benchmarks-flags-native INTERFACE -lc++fs -lc++experimental)
----------------
Can you record what's the goal of this patch in the commit message + Phab description?


================
Comment at: libcxx/benchmarks/CMakeLists.txt:106
 endif()
 
 add_library(           cxx-benchmarks-flags-libcxx INTERFACE)
----------------
Commit message typo: `Optmize`


================
Comment at: libcxx/include/__algorithm/ranges_max.h:1
 //===----------------------------------------------------------------------===//
 //
----------------
Separate patch: We should get rid of the `// TODO(ranges): ranges::min_element can now simply delegate to std::__min_element` todo comment.


================
Comment at: libcxx/include/__algorithm/ranges_max.h:12
 
+#include <__algorithm/ranges_min.h>
 #include <__algorithm/ranges_min_element.h>
----------------
That seems like a leftover.


================
Comment at: libcxx/include/__algorithm/ranges_max.h:71
 
-    if constexpr (forward_range<_Rp>) {
+    if constexpr (forward_range<_Rp> &&
+                  (!is_trivially_copyable_v<range_value_t<_Rp>> || sizeof(range_value_t<_Rp>) > 2 * sizeof(size_t))) {
----------------
Can you please make sure that both `ranges::min` and `ranges::max` have tests that call the algorithm on values that would be cheap to copy and non-cheap to copy? And also with iterators that would allow copying and disallow copying? We probably already do, but if we don't, let's make sure that we are testing this code path.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:13
 #include <__algorithm/ranges_min_element.h>
+#include <__algorithm/unwrap_range.h>
 #include <__assert>
----------------
This doesn't look used.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:67-68
     _LIBCPP_ASSERT(__first != __last, "range must contain at least one element");
-
-    if constexpr (forward_range<_Rp>) {
+    if constexpr (forward_range<_Rp> &&
+                  (!is_trivially_copyable_v<range_value_t<_Rp>> || sizeof(range_value_t<_Rp>) > 2 * sizeof(size_t))) {
       return *ranges::__min_element_impl(__first, __last, __comp, __proj);
----------------
```
// different header?
template <class _Tp>
constexpr bool __is_cheap_to_copy = is_trivially_copyable_v<_Tp> && sizeof(_Tp) <= sizeof(std::intmax_t) /* strawman proposal */;

// here
if constexpr (!__is_cheap_to_copy<range_value_t<_Rp>> && forward_range<_Rp>)
  use-min_element();
else
  use-the-naive-loop();
```

IMO this is easier to read and more self-explanatory.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143596