[libcxx-commits] [PATCH] D124122: [libc++] Optimize std::rotate

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I'd be fine with this since it addresses the underwhelming performance for simple types like `int`, which is super important. However, I would prefer if we instead went for a better algorithm directly, like the swap/grail rotate algorithm mentioned in https://github.com/llvm/llvm-project/issues/54949#issue-1206295098.



================
Comment at: libcxx/include/__algorithm/rotate.h:174
     typedef typename iterator_traits<_RandomAccessIterator>::value_type value_type;
-    if (is_trivially_move_assignable<value_type>::value)
+    if _LIBCPP_CONSTEXPR_AFTER_CXX14 (is_trivially_move_assignable<value_type>::value && sizeof(value_type) > 32)
     {
----------------
I don't think `constexpr` adds a lot of value here since the compiler will definitely fold this anyway, and it's kind of weird to have `_LIBCPP_CONSTEXPR_AFTER_CXX14` in that location.


================
Comment at: libcxx/include/__algorithm/rotate.h:176
     {
         if (_VSTD::next(__first) == __middle)
             return _VSTD::__rotate_left(__first, __last);
----------------
I would also do something like

```
const bool __is_expensive_to_move = sizeof(value_type) > 32;
if (is_trivially_foo<...> && __is_expensive_to_move) { ... }
```

That way, the code is somewhat self-documenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124122