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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

In D74397#1898087 <https://reviews.llvm.org/D74397#1898087>, @gchatelet wrote:

> @sivachandra it seems there's a problem with the `NAME` attribute in `add_entrypoint_object` as the symbol being exported in the final library is not `memcpy` anymore but the provided `NAME`
>
>    % nm /tmp/llvm-project_rel_compiled-with-clang/projects/libc/lib/libllvmlibc.a
>  
>   __errno_location.o:
>   0000000000000000 T __errno_location
>   0000000000000000 T _ZN11__llvm_libc16__errno_locationEv
>   0000000000000000 b _ZN11__llvm_libcL7__errnoE
>  
>   strcpy.o:
>                    U memcpy
>   0000000000000000 T strcpy
>                    U strlen
>   0000000000000000 T _ZN11__llvm_libc6strcpyEPcPKc
>  
>   strcat.o:
>   0000000000000000 T strcat
>                    U strlen
>   0000000000000000 T _ZN11__llvm_libc6strcatEPcPKc
>                    U _ZN11__llvm_libc6strcpyEPcPKc
>  
>   memcpy.o:
>   0000000000000000 A memcpy_x86_64_opt_avx512f
>   0000000000000000 T _ZN11__llvm_libc25memcpy_x86_64_opt_avx512fEPvPKvm
>   0000000000000000 T _ZN11__llvm_libc6memcpyEPvPKvm
>  
>   mmap.o:
>   0000000000000000 T mmap
>                    U _ZN11__llvm_libc16__errno_locationEv
>   0000000000000000 T _ZN11__llvm_libc4mmapEPvmiiil
>  
>   munmap.o:
>   0000000000000000 T munmap
>                    U _ZN11__llvm_libc16__errno_locationEv
>   0000000000000000 T _ZN11__llvm_libc6munmapEPvm
>  
>   raise.o:
>   0000000000000000 T raise
>   0000000000000000 T _ZN11__llvm_libc5raiseEi
>


I thought your use case was the other way around: you want to have multiple targets with the same entrypoint name. Is this not correct? The first argument to `add_entrypoint_object` is the target name (which is in sync with the CMake convention of using target names as the first arg.) The `NAME` option specifies the entrypoint name.



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:108
 #       DEPENDS <list of dependencies>
+#       COMPILE_OPTIONS <list of compile options>
+#       COMPILE_DEFINITIONS <list of compile definitions>
----------------
gchatelet wrote:
> @sivachandra this should be submitted as a separate patch but it's better to have context for the change.
This is fine.


================
Comment at: libc/src/string/CMakeLists.txt:55
+      memcpy
+      NAME ${memcpy_name}
+      SRCS memcpy.cpp
----------------
The `NAME` is the entrypoint name and not the target name. May be you mean the opposite here? As in, instead of `memcpy` on line 54, you want `${memcpy_name}` and `memcpy` for the `NAME` argument?


================
Comment at: libc/src/string/memcpy.cpp:15
+
+#ifdef DECLARE_LLVM_LIBC_ENTRYPOINT
+void *LLVM_LIBC_ENTRYPOINT(memcpy)(void *__restrict dst,
----------------
gchatelet wrote:
> We need to selectively declare the entry point depending on if we generate test or final implementation.
Test or final, all of them should have the same entrypoint name. No?

For example, `memcpy_config1_test` will depend on `memcpy_config1` but will call __llvm_libc::memcpy for the test.


================
Comment at: libc/src/string/memcpy_arch_specific.h.def:61
+#error "MEMCPY_NAME is not defined"
+#endif // MEMCPY_NAME
+
----------------
This should all be just `memcpy`.  May be I am missing something still.


================
Comment at: libc/test/src/string/CMakeLists.txt:32
+  add_libc_unittest(
+    ${memcpy_name}_test
+    SUITE
----------------
This seems to be setup correctly. But, instead of `memcpy_name`, they should be called `memcpy_config_name` to make it clear that they are all `memcpy` implementations in different configs.


================
Comment at: libc/test/src/string/CMakeLists.txt:38
+    DEPENDS
+      ${memcpy_name}
+    COMPILE_OPTIONS
----------------
This as well.


================
Comment at: libc/test/src/string/memcpy_test.cpp:12
 
+#ifndef MEMCPY_NAME
+#error "MEMCPY_NAME is not defined"
----------------
This should not be required.


================
Comment at: libc/test/src/string/memcpy_test.cpp:46
       char *const dst = &buffer[align];
-      __llvm_libc::memcpy(dst, src, count);
+      __llvm_libc::MEMCPY_NAME(dst, src, count);
       // Everything before copy is untouched.
----------------
This should also not be required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397