[libc-commits] [PATCH] D126768: [libc][mem*] Introduce Sized/Backends for new mem framework

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/src/string/memory_utils/backend_x86.h:43
+  static inline T load(const T *src) {
+    DeferredStaticAssert("X86 non-temporal load needs aligned access");
+    return {};
----------------
"unspecialized case". And the function should have a comment saying something like:

// This function is specialized below.

And specialize it for non-temporal loads if you want a better user message.


================
Comment at: libc/src/string/memory_utils/backend_x86.h:56
+  static inline void store(T *dst, T value) {
+    DeferredStaticAssert("X86 non-temporal store needs aligned access");
+  }
----------------
ditto



================
Comment at: libc/src/string/memory_utils/backend_x86.h:59
+
+  template <typename T> static inline T splat(ubyte value) {
+    return ScalarBackend::template splat<T>(value);
----------------
// Forwarding to base class because c++ does not allow specializing base class methods.


================
Comment at: libc/src/string/memory_utils/backend_x86.h:69
+            cpp::EnableIfType<ScalarBackend::IsScalarType<T>, bool> = true>
+  static inline int32_t three_way_cmp(T v1, T v2) {
+    return ScalarBackend::template three_way_cmp<T>(v1, v2);
----------------
I've just noticed that all functions in this patch should be using `threeWayCmp` case...


================
Comment at: libc/src/string/memory_utils/backend_x86.h:81
+  template <size_t Size>
+  using getNextType = cpp::ConditionalType<
+      (HAS_M512 && Size >= 64), __m512i,
----------------
doc?


================
Comment at: libc/src/string/memory_utils/backend_x86.h:127
+inline uint64_t X86Backend::not_equals<__m128i>(__m128i a, __m128i b) {
+  using T = char __attribute__((__vector_size__(16)));
+  return _mm_movemask_epi8(T(a) != T(b));
----------------
why not `_mm_cmpeq_epi8` instead ?


================
Comment at: libc/src/string/memory_utils/backend_x86.h:148
+inline uint64_t X86Backend::not_equals<__m256i>(__m256i a, __m256i b) {
+  using T = char __attribute__((__vector_size__(32)));
+  return _mm256_movemask_epi8(T(a) != T(b));
----------------
why not `_mm256_cmpeq_epi8` instead ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126768