[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/cmake/modules/LLVMLibCRules.cmake:149
   )
+  if(ADD_ENTRYPOINT_OBJ_COMPILE_OPTIONS)
+    target_compile_options(
----------------
sivachandra wrote:
> Sorry for not mentioning this earlier. I have copied this part in to another patch of mine.
Yes, I think I messed up with the merge, should be fixed.


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:155
+  endif()
+  if(ADD_ENTRYPOINT_OBJ_COMPILE_DEFINITIONS)
+    target_compile_definitions(
----------------
sivachandra wrote:
> Do we need this still?
Right now, support for `repmovsb` is disabled. This is a customization point that should be exposed through preprocessor definitions (and CMake options).
As of this patch this is not needed but I expect to need it soon-ish.


================
Comment at: libc/src/string/CMakeLists.txt:45
+
+foreach(opt_level ${LIBC_MEMCPY_OPT_LEVELS})
+  generate_function_name(memcpy ${LIBC_TARGET_MACHINE} ${opt_level} memcpy_name)
----------------
sivachandra wrote:
> Should we check if `${opt_level}` is available for the machine this target is being built?
Yes, makes sense.
I'm still not super happy about the introspection part, it's cumbersome to use...
I'll work on it as a separate patch if you don't mind.


================
Comment at: libc/test/src/string/memcpy_test.cpp:12
+
+namespace __llvm_libc {
+
----------------
sivachandra wrote:
> Instead of this, you can include `src/string/memcpy.h`.
Ha sure !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397