[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/README.md:27
+    if (count == 3) return Memset<3>::block(dst, value);
+    if (count <= 8) return Memset<4>::head_tail(dst, value, count);
+    if (count <= 16) return Memset<8>::head_tail(dst, value, count);
----------------
`// Note that 0 to 4 bytes are written twice.`


================
Comment at: libc/src/string/memory_utils/README.md:68
+
+## Scoped implementations
+
----------------
I would say "specializations"


================
Comment at: libc/src/string/memory_utils/README.md:87
+
+Ultimately we would like the compiler to provide the code for the `block` function. For this we rely on dedicated builtins available in Clang (e.g., [`__builtin_memcpy_inline`](https://clang.llvm.org/docs/LanguageExtensions.html#guaranteed-inlined-copy), [`__builtin_memset_inline`](https://clang.llvm.org/docs/LanguageExtensions.html#guaranteed-inlined-memset))
+
----------------
`__builtin_memset_inline`, given that you're talking about `memset` here.


================
Comment at: libc/src/string/memory_utils/op_generic.h:19
+// On the other hand, if the platform is x86 with support for AVX the maximum
+// size is 32 and the operation can be handled at once.
+//
----------------
"with a single native operation" ?


================
Comment at: libc/src/string/memory_utils/op_generic.h:47
+};
+template <typename... Ps> struct CTMap : public Ps... {
+  using Ps::get_pair...;
----------------
`Pairs` ?


================
Comment at: libc/src/string/memory_utils/op_generic.h:66
+  }
+  static inline void splat(Ptr dst, uint8_t value) {
+    Type splatted_value = Type(~0) / Type(0xFF) * Type(value);
----------------
Any reason fro `splat` to store ? I would expect `static inline Type splat(uint8_t value)`.


================
Comment at: libc/src/string/memory_utils/op_generic.h:94
+                            CTPair<4, ScalarType<uint32_t>>, //
+                            CTPair<8, ScalarType<uint64_t>>, //
+                            CTPair<16, VectorType<16>>,      //
----------------
x86 + SSE does not have this.


================
Comment at: libc/src/string/memory_utils/op_generic.h:99
+
+// Implements load, store and splat for sizes not natively supporter by the
+// platform. SubType is either ScalarType or VectorType.
----------------
d


================
Comment at: libc/src/string/memory_utils/op_generic.h:127
+// Compute the type to handle an operation of Size bytes knowing that the
+// underlying platform only support native types up to MaxSize bytes.
+template <size_t Size, size_t MaxSize>
----------------
Support is not necessarily a linear range of powers of `2`: e.g. x86+SSE which has 1,2,4,16


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135134