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

[Cheng Wang via Phabricator via libc-commits]

cheng.w marked 4 inline comments as done.
cheng.w added a comment.

Address comments.



================
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);
----------------
gchatelet wrote:
> 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.
It should be careful with pointer operations and limits. Is assert more like a debug or test macro?


================
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);
----------------
gchatelet wrote:
> 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.
Yes, it is verbose. Maybe inline asm like `cld/std, rep movsb` would help. I leave a TODO here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93195