[libc-commits] [PATCH] D74397: [libc] Adding memcpy implementation for x86_64

[Alex Brachet via Phabricator via libc-commits]

abrachet added a subscriber: ckennelly.
abrachet added a comment.

I've gone ahead and copy and pasted some of this into godbolt to save others some time if they wish to play around https://godbolt.org/z/z4dCmj :)

> How do we build? We may want to test in debug but build the libc with -march=native for instance,

This is logical. To my knowledge we don't currently do anything special when CMAKE_BUILD_TYPE=Release but this makes sense to turn on for release and benchmarking.

> With gcc we can use __builtin_memcpy but then we'd need a postprocess step to check that the final assembly do not contain call to memcpy (unlikely but allowed),

I think we can do this in cmake with `add_custom_command` and `POST_BUILD` specified then just `nm $<TARGET_FILE:target> | grep "U memcpy"`



================
Comment at: libc/src/string/memcpy_x86_64.cpp:12
+#include "src/string/memory_utils/memcpy_utils.h"
+
+namespace __llvm_libc {
----------------
It might be worth it as a sanity check because this is an x86 specific file to static_assert LLVM_LIBC_CACHELINE_SIZE == 64


================
Comment at: libc/src/string/memcpy_x86_64.cpp:21-30
+  if (count == 0)
+    return;
+  if (count == 1)
+    return Copy<1>(dst, src);
+  if (count == 2)
+    return Copy<2>(dst, src);
+  if (count == 3)
----------------
Off of intuition I would imagine these are very rare sizes to call `memcpy` with. Would it be better to do something like `if (count < 5) goto smallCount;` and move these down?
It's worth linking while we are here @ckennelly's [[ http://lists.llvm.org/pipermail/libc-dev/2020-January/000032.html | thread ]] from a few weeks ago. "For `memcpy`, 96% of sizes are <= 128 bytes.  99% are <= 1024 bytes."


================
Comment at: libc/src/string/memcpy_x86_64.cpp:44
+  // else CopyAligned is used to to kRepMovsBSize and then RepMovsb.
+  constexpr size_t kRepMovsBSize = -1;
+  if (count <= kRepMovsBSize)
----------------
I'm not sure I'm understanding this. It's not like we can change this at compile time, line 47 is dead code.


================
Comment at: libc/src/string/memcpy_x86_64.cpp:46
+  if (count <= kRepMovsBSize)
+    return CopyAligned<32>(dst, src, count);
+  CopyRepMovsb(dst, src, count);
----------------
Why was 32 chosen?


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
D74091 Was also wanting to use `assert`/`abort` we should add them. Also that comment is funny to me because its the only thing in assert.h :)


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:17
+
+#include <cstdio> // printf
+
----------------
Not used as far as I can tell


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397