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

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

> See above, the generated code is improved (smaller and faster) when targeting specific architecture.
>  glibc is using IFUNC to that matter and lets the runtime pick the best implementation.
>  Although feasible in llvm-libc we noticed that the required extra level of indirection is hurting branch prediction and latency for small sizes (which are the most frequent)

The resolver function is only called the first time that ld.so binds the symbol into the plt so I don't think it would hurt branch prediction any more than any other dynamic symbol. Notably a big goal of llvm libc stated from Siva early on was static linking, so I am sure we will end up having a build time way of determining which specialization to call, as well as runtime with ifunc.

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

Were you asking how to express this in CMake?

  set_property(
    SOURCE memcpy.cpp
    PROPERTY COMPILE_OPTIONS
    -fno-builtin-memcpy
  )

This worked in my (quick) testing. `ninja -v` will show the commands it's running so you can see if it worked here too.



================
Comment at: libc/src/string/memcpy_arch_specific.h.def:21
+
+#endif // #define LLVM_LIBC_SRC_STRING_MEMORY_ARCH_H
----------------
remove `#define`


================
Comment at: libc/src/string/memory_utils/memcpy_utils.h:22
+#elif defined(__GNUC__)
+#if __has_builtin(__builtin_memcpy_inline)
+#define USE_BUILTIN_MEMCPY
----------------
Presumably should be just `__builtin_memcpy`


================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
sivachandra wrote:
> 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.
> The abort function is fairly involved and needs other pieces to be in place for us to build llvm-libc's implementation.
I was actually working on writing to the list about getting started on signals, which is how abort should be implemented I think, `for(;;) raise(SIGABRT);`. I'd be happy to get started on signals :)

I think for now though, it is fine to have abort be `__builtin_trap()` because we are mainly focused on x86 at the moment. (I remember on ARM32 __builtin_trap() calls abort for example)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397