[libc-commits] [PATCH] D93195: [libc] Add memmove implementation.

[Guillaume Chatelet via Phabricator via libc-commits]

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

My apologies for the late reply. I've made a bunch of comments.



================
Comment at: libc/src/string/memmove.cpp:21
+                             size_t count, ssize_t direction) {
+  for (ssize_t offset = 0; count; --count, offset += direction)
+    *(dest_m + offset) = *(src_m + offset);
----------------
`assert(count <= SSIZE_MAX);`

@sivachandra I don't remember if we allow asserts in the library?


================
Comment at: libc/src/string/memmove.cpp:22
+  for (ssize_t offset = 0; count; --count, offset += direction)
+    *(dest_m + offset) = *(src_m + offset);
+}
----------------
nit: subscript operator is clearer IMHO `dest_m[offset] = src_m[offset]`


================
Comment at: libc/src/string/memmove.cpp:37
+  // Use memcpy if src_c and dest_c do not overlap.
+  if (__llvm_libc::labs(src_c - dest_c) >= static_cast<long>(count))
+    return __llvm_libc::memcpy(dest_c, src_c, count);
----------------
Technically the pointer difference type is `ptrdiff_t`, you may want to rewrite this as:

```
#include "src/stdlib/abs_utils.h" // __llvm_libc::integer_abs
#include <cstdint> // ptrdiff_t, PTRDIFF_MAX
...
assert(count <= PTRDIFF_MAX);
if (__llvm_libc::integer_abs(src_c - dest_c) >= static_cast<ptrdiff_t>(count))
```

Also update the comment above.


================
Comment at: libc/src/string/memmove.cpp:52-55
+  if (dest_c < src_c)
+    move_byte(dest_c, src_c, count, /*pointer add*/ 1);
+  if (dest_c > src_c)
+    move_byte(dest_c + count - 1, src_c + count - 1, count, /*pointer add*/ -1);
----------------
The code generated here seems suboptimal and quite large https://godbolt.org/z/fMnfv8.
This can be bad for icache pressure (i.e. calling `memmove` from time to time will evict precious cache lines from iL1).

That said, we can keep this version for now and optimize later on.


================
Comment at: libc/test/src/string/memmove_test.cpp:42
+TEST_F(MemmoveTest, OverlapThatDestStartsBeforeSrc) {
+  // Set boundary at benginning and end for not overstepping when
+  // copy forward or backward.
----------------
typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93195