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

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/string/CMakeLists.txt:10
+  COMMENT "Check that the generated code is valid"
+  COMMAND ! (nm --undefined-only $<TARGET_PROPERTY:${target_name},OBJECT_FILE_RAW> | grep U && echo "Implementation contains undefined symbols") 
+)
----------------
[[ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-nm/llvm-nm.cpp#L811 | llvm-nm ]] (the nm that ships with MacOS) for mach-o files prints just the symbol and not its value or type character so grepping for U wouldn't work here (in this very niche case that I just happened to test). If we're using --undefined-only we could just `grep .` perhaps?


================
Comment at: libc/src/string/CMakeLists.txt:78
+  compile_memcpy_with_flags(avx512 FLAGS -mavx512f)
+endif()
+
----------------
sivachandra wrote:
> gchatelet wrote:
> > This now creates the following memcpy implementations
> >  - `__llvm_libc::memcpy_x86_64_avx512`
> >  - `__llvm_libc::memcpy_x86_64_avx`
> >  - `__llvm_libc::memcpy_x86_64_sse2`
> >  - `__llvm_libc::memcpy_x86_64_sse`
> >  - `__llvm_libc::memcpy_x86_64_unopt`
> > 
> > For shared libc, we need an ifunc like trampoline to select the correct version.
> > For static libc, we need to select an implementation
> > 
> > @sivachandra how do you see this kind if code generation integrate into the more general cmake functions from `libc/cmake/modules/LLVMLibCRules.cmake`?
> > I expect other memory functions to follow the same scheme.
> Instead of building all the possible implementations, could we use the CMake [[ https://cmake.org/cmake/help/v3.14/command/try_compile.html | `try_compile` ]] and/or [[ https://cmake.org/cmake/help/v3.14/command/try_run.html | `try_run` ]] command to sniff out the best flags to use? I think `try_run` is more appropriate as I expect that we need to run the `cpuid` instruction? Also, compilers have a convenience macro `__cpuid` to run this instruction on x86/x86_64?
> 
> BTW, one can have ifuncs in static libraries as well. But, I do understand we want to avoid the overhead of the added indirection, so sniffing out at configure time is the best. If we can setup something for configure time sniffing, I believe we should be able use it (may be with straightforward extension/modification iff required) to use as the ifunc selector as well.
I think this is going to get quickly outside of the scope of this patch. It would probably be better to work on this in a separate patch.


================
Comment at: libc/src/string/memcpy_arch_specific.h.def:33-34
+// - As compilers and processors get better, the generated code is improved
+// with
+//   little change on the code side.
+static void memcpy_no_return(char *__restrict dst, const char *__restrict src,
----------------
Seems like clang-format interfered a little bit here.


================
Comment at: libc/src/string/memcpy_entrypoint.cpp:1
+//===--------------------- Implementation of memcpy -----------------------===//
+//
----------------
Must this be in its own file? Why not in src/string/memcpy.cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397