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

[Marco Gelmi via Phabricator via libcxx-commits]

marcogelmi marked an inline comment as done.
marcogelmi added a comment.

In D118029#3335871 <https://reviews.llvm.org/D118029#3335871>, @Quuxplusone wrote:

> FWIW, I have no strong desire to be obstructionist, and I definitely don't think we should //abandon// this direction, but I have one bad and one good(?) reason I think we should hold this PR in suspended animation for a while longer:
>
> - Bad: I'm still uncomfortable with the invasiveness of the changes to `robust_against_copying_comparators.pass.cpp`.
> - Good(?): Very soon now we're going to have to implement `ranges::sort`, which innovates in a bunch of different directions — we're going to have to deal with sentinels as well as iterators, and we're going to have to replace `__c(*x, *y)` with `invoke(__c, invoke(__p, *x), invoke(__p, *y)`, and so on. I feel like it would be a good idea to get //that// stuff out of the way first, and then revisit algorithmic changes like this PR, rather than vice versa. Unfortunately I have no particular estimate of when `ranges::sort` will get done; @philnik might have a long-range plan to share, but being as we're still down in the foothills of `ranges::min_element` etc., personally I'd predict it'll be a couple months //at least//.
>
> Anyway, @ldionne will be the ultimate arbiter (but IIRC he's on vacation this week).
>
> I'll also keep recommending that you replace `is_arithmetic` with `is_scalar`, until you actually do it! or explain why not. :)

Regarding `is_arithmetic` vs `is_scalar`: 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:
> marcogelmi wrote:
> > 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
> For my curiosity, why did you have to change to a plain enum? (All else being equal, we should prefer `enum class`, but if it really technically doesn't work, then I don't imagine it's a big deal.)
this is just because we need to cast it to `int` to index into the `Types` tuple and that's more tricky with an `enum class`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118029