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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

> The `rep movsb` instruction performance is highly tied to the targeted microarchitecture.
>  For one there is the ERMS cpuid flag https://en.wikipedia.org/wiki/CPUID#EAX=7,_ECX=0:_Extended_Features that helps to know if it should be used at all.
>  Then depending on the microarchitecture the crosspoint between aligned copy and `rep movsb` varies between 512 to a few kilobytes.
>  Ideally we need to adapt this threshold somehow to provide the best implementation.
> 
> Eventually, when `rep movsb` becomes excellent we can replace the function entirely with this single instruction.
> 
> understand that `llvm-libc` is to be a pick and choose what you need libc which implies some sort of customization anyways. Am I right?

Would configure time sniffing to detect what is available work? It will not work in case of cross-compilation. But, requiring explicit setting of build params for cross-compilation seems OK to me.

>> A related question: considering we are using `__builtin_memcpy` and inline assembly, does the code work as is with MSVC?
> 
> No it doesn't, I can add a fallback case if you want but the generated code is not good right now.
>  This means we'd need to specialize the `CopyRepMovsb` function so it uses the correct syntax for msvc `__asm` instead of the provided `LIBC_INLINE_ASM`

We don't need to provide the specialization in this patch, but pointing out which parts needs to be specialized would help.

> Quick question @sivachandra , how do we currently build `llvm-libc`?
>  `utils/build_scripts` https://github.com/llvm/llvm-project/blob/master/libc/docs/source_layout.rst#the-utilsbuild_scripts-directory seems to be missing so  I could only build the entrypoints via transitive dependency through tests.

Sorry, that file is out of date. Will fix it soon.
Add the target of your entrypoint to the list of `DEPENDS` for the `llvmlibc` target here: https://github.com/llvm/llvm-project/blob/master/libc/lib/CMakeLists.txt
Running `ninja llvmlibc` after that will produce `libllvmlibc.a` which includes your entrypoint.



================
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"
----------------
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
```


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
gchatelet wrote:
> sivachandra wrote:
> > 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.
> I think that would be nice to have the functions in `memcpy_utils.h` have some precondition checking, an `abort` function would be useful indeed,
> ```
> #if not defined(NDEBUG)
> // check precondition
> #endif
> 
> ```
Okay. There are two kinds of `abort` here. One to use in the implementation, another to use in the test. For the `abort` used in the test, we should use the one coming from the system libc. For the one going into the implementation, we should use the one from llvm-libc. The abort function is fairly involved and needs other pieces to be in place for us to build llvm-libc's implementation. Let me do my homework get back to you on that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397