[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/LLVMLibCCheckCpuFeatures.cmake:10
+list(SORT ALL_CPU_FEATURES)
+message(STATUS "Available Cpu Features: ${ALL_CPU_FEATURES}")
+
----------------
abrachet wrote:
> Is it necessary to print this?
Not really.


================
Comment at: libc/cmake/modules/cpu_features/check_cpu_features.cpp.in:4-10
+// Handle MSVC SSE
+#if (defined(_M_AMD64) || defined(_M_X64) || (_M_IX86_FP == 2))
+#define __SSE2__ 1
+#define __SSE__ 1
+#elif _M_IX86_FP == 1
+#define __SSE__ 1
+#endif
----------------
abrachet wrote:
> Is it necessary to define these? Does DEFINITIONS rely on the definitions of macros? Also I think handling MSVC here is strange when `compute_flags` does not currently.
Right, it's probably better to defer anything related to MSVC at that point.
Also I'll add documentation to this file because it's a bit obscure.


================
Comment at: libc/cmake/modules/cpu_features/check_cpu_features.cpp.in:19-20
+    if (i)
+      putc(';', stdout);
+    fputs(strings[i], stdout);
+  }
----------------
abrachet wrote:
> Why not `putchar` and `puts`?
I used `putchar` for a single character but I can't use `puts` since it automatically appends a line feed.


================
Comment at: libc/test/src/string/CMakeLists.txt:32
+  if(can_run)
+    compute_flags(flags MARCH native)
+    add_libc_unittest(
----------------
abrachet wrote:
> Would you mind explaining this? It seems like ${flags} will just be -march=native, and the work above to find flags gets ignored.
Actually there's no need for flags here, the implementation has already been compiled with the correct flags. The test itself doesn't need them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397