[libcxx-commits] [PATCH] D118029: Introduce branchless sorting functions for sort3, sort4 and sort5.

[Marco Gelmi via Phabricator via libcxx-commits]

marcogelmi marked 5 inline comments as done.
marcogelmi added a comment.

Thanks for the suggestions.

Regarding `is_scalar` vs `is_arithmetic`: expanding to `is_scalar` would include pointer types (which are likely to have a custom comparator) so we would have to test whether our changes provide similar improvements when you have a complex custom comparator. This is also not trivial to do with the current test setup.

Can we therefore first submit this CL and then look into the feasibility of incorporating those additional types into our setup?



================
Comment at: libcxx/benchmarks/algorithms.bench.cpp:30-31
             V() == ValueType::Pair, std::pair<uint32_t, uint32_t>,
-            std::conditional_t<V() == ValueType::Tuple,
-                               std::tuple<uint32_t, uint64_t, uint32_t>,
-                               std::string> > > >;
+            std::conditional_t< V() == ValueType::Tuple, std::tuple<uint32_t, uint64_t, uint32_t>,
+                                std::conditional_t< V() == ValueType::String, std::string, float> > > > >;
 
----------------
Quuxplusone wrote:
> philnik wrote:
> > 
> It might be (long past) time to change this to simply
> ```
> using Types = std::tuple<
>     uint32_t, uint64_t, std::pair<uint32_t, uint64_t>, std::tuple<uint32_t, uint64_t, uint32_t>,
>     std::string, float
> >;
> 
> template<class V>
> using Value = std::tuple_element_t<(int)V(), Types>;
> ```
note that I'm using clang-format and it's changing the format slightly. Also, I had to change to a plain enum


================
Comment at: libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp:177
+    (void)std::set_union(first, mid, mid, last, first2, Less<T>(&copies)); assert(copies == 0);
+    (void)std::sort(first, last, Less<T>(&copies)); assert(copies == 0);
+    (void)std::sort_heap(first, last, Less<T>(&copies)); assert(copies == 0);
----------------
Quuxplusone wrote:
> IIUC, what you've done here is just add a call to `std::sort` with `value_type=int`, which triggers your new thing, specifically because your new thing is constrained to `arithmetic` value_types instead of (as I keep trying to drive-by it into) `scalar` value_types. (Pointer types are scalar but not arithmetic.)
> 
> Is this the only change that was needed in order to trigger your comparator-copying codepath?
> If so, I'd slightly rather see you just expand your new thing to include `scalar` types!
> 
> I don't really object to the idea of templatizing the whole thing on `T` and testing with different `T`s; but it feels like a lot of churn if the //only// culprit was `arithmetic/scalar`, and also I'm skeptical that that //was// the only culprit here.
> 
> I had thought that the problem was more about the shape of the input data — I mean, notice that `is_sorted(first, last)` is already true going into this call.
> 
> In particular, if we now know that there are different codepaths taken for arrays of length 3, 4, 5, then shouldn't we be testing here
> ```
> (void)std::sort(first, first+3, Less(&copies)); assert(copies == 0);
> (void)std::sort(first, first+4, Less(&copies)); assert(copies == 0);
> (void)std::sort(first, first+5, Less(&copies)); assert(copies == 0);
> (void)std::sort(first, last, Less(&copies)); assert(copies == 0);
> ```
> ?
yes this is triggered by `int` specifically because `void*` is not included by `is_arithmetic`

the reason why this is triggered even if the shape of the data is of length 10 is because there is a base case (`insertion_sort_3`) which calls `__sort3` once and is invoked when 5 < size <= 30.

Anyway, I've added the test cases you recommended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118029