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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM. Few minor questions inline.



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:149
   )
+  if(ADD_ENTRYPOINT_OBJ_COMPILE_OPTIONS)
+    target_compile_options(
----------------
Sorry for not mentioning this earlier. I have copied this part in to another patch of mine.


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:155
+  endif()
+  if(ADD_ENTRYPOINT_OBJ_COMPILE_DEFINITIONS)
+    target_compile_definitions(
----------------
Do we need this still?


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:324
+#      COMPILE_OPTIONS <list of compile options>
+#      COMPILE_DEFINITIONS <list of compile definitions>
 #    )
----------------
Likewise, seems to me like `COMPILE_DEFINITIONS` is not used.


================
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)
----------------
Should we check if `${opt_level}` is available for the machine this target is being built?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397