[libc-commits] [PATCH] D135134: [libc] New version of the mem* framework

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/src/string/memory_utils/op_aarch64.h:36
+  static inline void block(Ptr dst, uint8_t) {
+    static_assert(Size == 64);
+#if __SIZEOF_POINTER__ == 4
----------------
Does the class need to be a template ?


================
Comment at: libc/src/string/memory_utils/op_aarch64.h:81
+
+  static inline BcmpType block(CPtr p1, CPtr p2) {
+    if constexpr (Size == 32) {
----------------
you might want to get someone more familiar with Aarch64 to review this part.


================
Comment at: libc/src/string/memory_utils/op_generic.h:93
+          CTPair<2, ScalarType<uint16_t>>, //
+          CTPair<4, ScalarType<uint32_t>>, //
+#if !defined(LLVM_LIBC_ARCH_X86_32)        //
----------------
Do we support any non-32-bit platform ? e.g. 8- ot 16- bit ones ?


================
Comment at: libc/src/string/memory_utils/op_generic.h:94
+          CTPair<4, ScalarType<uint32_t>>, //
+#if !defined(LLVM_LIBC_ARCH_X86_32)        //
+          CTPair<8, ScalarType<uint64_t>>, // Not available on 32bit
----------------
Should we conditionally enable on platforms we *know* support 64 bit natively, rather than conditionally disable ?


================
Comment at: libc/src/string/memory_utils/op_x86.h:106
+  const int mask = _mm256_movemask_epi8(load<T>(p1) != load<T>(p2));
+  return static_cast<uint32_t>(mask);
+}
----------------
please document why the `static_cast` here is fine.


================
Comment at: libc/src/string/memory_utils/op_x86.h:116
+  const uint64_t mask = _mm512_cmpneq_epi8_mask(load<T>(p1), load<T>(p2));
+  const uint32_t mask_is_set = mask != 0;
+  return mask_is_set;
----------------
maybe make all casts from `bool` to `uint32_t` explicit ?


================
Comment at: libc/src/string/memory_utils/op_x86.h:180
+  using T = char __attribute__((__vector_size__(16)));
+  // A mask indicating which byte differ after loading 16 bytes from p1 and p2.
+  if (int mask = _mm_movemask_epi8(load<T>(p1) != load<T>(p2)))
----------------
bytes


================
Comment at: libc/src/string/memory_utils/utils.h:128
+using MemcmpType = StrictIntegralType<int32_t>;
+using BcmpType = StrictIntegralType<uint32_t>;
+
----------------
Technically `bcmp` and `memcmp` return `int`, not `uint32_t`, so you need to check that `sizeof(uint32_t) <= sizeof(int)` on the platform, and the final `bcmp` implementation `LLVM_LIBC_FUNCTION(int, bcmp)` should bitcast to `int`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135134