[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`

Tue Sep 20 19:14:00 PDT 2022

var-const added a comment.

In D130695#3783138 <https://reviews.llvm.org/D130695#3783138>, @philnik wrote:

> Sorry, I know I approved it before, but I just noticed that this patch still regresses on some optimizations (in particular optimizing `reverse_iterator<contiguous-iterator>`), so maybe I'm missing more and I'm also questioning the usefulness of the changes more and more, so I'd like some clarification. What exactly is the goal of the refactoring here? How does the refactored code do the job better than forwarding `copy_backward`, `move` and `move_backward` to `copy`?  The answers to these questions would probably also be good to have in the commit message. I first thought that his patch removed some of the complication in `copy.h`, but the longer I think about it it the more it looks to me like it's just shifting the complexity around.

The main motivation is to reduce complexity and remove code duplication. Majority of the remaining complexity is due to having to support C++03. The ideal state I envision is essentially something like

  if constexpr (__can_memmove<_InIter, _OutIter>) {
    return __memmove(__first, __last, __out);
  } else {
    return __copy_loop(__first, __last, __out);
  }  

in every algorithm (as a side note, I really hope we get Clang to support `if constexpr` in C++03 mode at some point). This patch brings this as close to that state as I could within the limitations of C++03.

As for specific improvements:

- the optimization was fully duplicated between `std::copy` and `std::move`. Moreover, `std::move_backward` had a divergent, less thorough implementation of the optimization. Now the duplication is removed and everything related to this optimization is encapsulated in one place.
- all 4 algorithms now have symmetrical, self-contained implementations which makes them easier to read and debug.
- `reverse_iterator` is no longer used in the implementation, which makes it significantly easier to understand and removes artificial types from backtraces. Using `reverse_iterator` has lead to at least one serious bug already.
- `unconstrainted_reverse_iterator` can now be removed (which I intend to do in a follow-up), removing a lot of code duplication and complexity.
- the test for whether the optimization is used only applied to `std::copy` and, more importantly, was essentially a no-op because it would still pass if the optimization was not used;
- there were no tests to make sure the optimization is not used when the effect would be visible.

Regarding the `reverse_iterator` optimization you mention, I think it was solution for a problem that the previous approach introduced and that is no longer applicable. The optimization was very limited in that it would only apply to the case where both the input and the output iterators were reverse iterators. I see no inherent reason why users would reverse both the input and the output sequence rather than just one of them, outside of the artificial case where our implementation itself would do the wrapping. If this optimization were to be restored, it would have to apply equally to input and output iterators. I don’t think, however, it would be pulling its weight.

================
Comment at: libcxx/include/__algorithm/move.h:32
+    while (__first != __last) {
+      *__result = _IterOps<_AlgPolicy>::__iter_move(__first);
+      ++__first;
----------------
ldionne wrote:
> var-const wrote:
> > huixie90 wrote:
> > > question: if the input is  a contiguous iterator pointing to a trivially copyable/moveable type, but the iterator has specialised `iter_move` ( yes it is very contrived), would the `move` dispatch to the overload that unwraps the  contiguous iterator to pointer and call `memmove` without going through the specialised `iter_move`?
> > I think this is a great point, thanks. I'm inclined to leave it as is for this patch because it's a preexistent thing, but I will address it in a follow-up.
> Is this really something we should fix? Aren't we allowed to bypass the iterator's `iter_move`?
Hmm, inlining the definition from the standard a little bit, it looks like:
```
Effects: [...] For each non-negative integer `n < N`, performs `*(result + n) = ranges::iter_move(first + n)`.
```
I'm not sure if there's anything that would allow us to bypass this?
(see https://eel.is/c++draft/alg.move)

================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:25
+
+template <size_t N, class Iter, std::enable_if_t<N == 0>* = nullptr>
+constexpr auto wrap_n_times(Iter i) {
----------------
philnik wrote:
> Maybe use a `requires` since the test requires C++20 anyways?
Good point.

================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:23
+#include <type_traits>
+
+struct Foo {
----------------
philnik wrote:
> To make it less prone to failing when we change includes, could you move the `<type_traits>` include down and `static_assert` the `trivially_copyable` later? It should still catch a problem before anything unpredictable happens. AFAICT you don't actually need the `<cstring>` include. `<cstddef>` should suffice for `size_t`.
Replaced the `<cstring>` include (it's from the previous iteration which used `memmove`). I'd prefer to keep the static assertion close to the point of class definition, though. Can you elaborate on how the test might fail if we change includes?

================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_pointer_size.pass.cpp:9-11
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=2000000
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=2000000
+
----------------
philnik wrote:
> Why is this required? Did it hit the constexpr limit without the `__builtin_memmove`? In that case, please remove the bumped limit again.
It failed on CI. I'll remove the flags and see if the current state stays within the limit or not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695