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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/src/string/memory_utils/memcpy_utils.h:22
+#elif defined(__GNUC__)
+#if __has_builtin(__builtin_memcpy_inline)
+#define USE_BUILTIN_MEMCPY
----------------
gchatelet wrote:
> abrachet wrote:
> > Presumably should be just `__builtin_memcpy`
> Good catch
`__has_builtin` is actually fairly new and `__builtin_memcpy` pretty old so I'm just going to assume that gcc has `__builtin_memcpy`.


================
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"
----------------
sivachandra wrote:
> gchatelet wrote:
> > sivachandra wrote:
> > > Where is `monitor_memcpy` defined?
> > Further down after the `GetTrace` function.
> Ah sorry, I missed it. So, does it mean that you want that name to be overridable/customizable? If yes, then would it make sense to define it to `monitor_memcpy` only if not defined?
> 
> ```
> #ifndef LLVM_LIBC_MEMCPY_MONITOR
> #define LLVM_LIBC_MEMCPY_MONITOR monitor_memcpy
> #endif
> ```
I've defined it in the `CMakeLists.txt` with a check in the test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397