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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

> How do we customize the implementation? (i.e. how to define `kRepMovsBSize`),

What kind of customizations?

> - How do we specify custom compilation flags? (We'd need `-fno-builtin-memcpy` to be passed in),

Do we need to pass this flag for building llvm-libc, or to user code (and llvm-libc tests?) Reading the code, it seems to me that it is the latter, correct?

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

Not sure I understand this fully. What are the use cases/goals of what you are describing here?

> Clang has a brand new builtin `__builtin_memcpy_inline` which makes the implementation easy and efficient, but:
> 
> - If we compile with `gcc` or `msvc` we can't use it, resorting on less efficient code generation,

Less efficient wrt clang compiled code? Can we rephrase what you are saying as, "we make use of a better optimization when compiled with clang?"

> - 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),

The concern is that it will become a recursive call? What action is to be taken if we do find a call to `memcpy` in the final assembly?

> - For msvc we'd need to resort on the compiler optimization passes.

Does "we" mean llvm-libc developers? A related question: considering we are using `__builtin_memcpy` and inline assembly, does the code work as is with MSVC?



================
Comment at: libc/src/string/memcpy_x86_64.cpp:15
+
+static void CopyRepMovsb(char *dst, const char *src, size_t count) {
+  LIBC_INLINE_ASM("rep movsb" : "+c"(count), "+S"(src), "+D"(dst) : : "memory");
----------------
Is this the only reason this file name has the `_86_64` in it? If yes, is this function the only machine specific piece for other archs as well? If yes, we should put it in a header file and use the same scheme we use for `LLVM_LIBC_CACHELINE_SIZE`.


================
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)
----------------
abrachet wrote:
> I'm not sure I'm understanding this. It's not like we can change this at compile time, line 47 is dead code.
I guess this relates to the customization @gchatelet mentioned about in the patch description.


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:9
+
+#define LLVM_LIBC_MEMPCY_MONITOR monitor_memcpy
+#include "src/string/memory_utils/memcpy_utils.h"
----------------
Where is `monitor_memcpy` defined?


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
abrachet wrote:
> 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 :)
Point taken. I will prepare something to address this.


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:15
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
+
----------------
Use `stdint.h` instead of `cstdint`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397