[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:1
+#ifndef LLVM_LIBC_SRC_STRING_MEMORY_UTILS_BACKEND_X86_H
+#define LLVM_LIBC_SRC_STRING_MEMORY_UTILS_BACKEND_X86_H
----------------
Missing header (same above).


================
Comment at: libc/src/string/memory_utils/backends.h:12
+// selection. It defines how to implement aligned/unaligned,
+// temporal/non-temporal loads and stores for a particular architecture as well
+// as efficient ways to fill and compare types.
----------------
"native loads and stores", right ? 


================
Comment at: libc/src/string/memory_utils/backends.h:36
+
+  // return a T filled with `value`
+  template <typename T> static T splat(ubyte value);
----------------
[Nit]sentences.


================
Comment at: libc/src/string/memory_utils/sized_op.h:11
+// framework. It implements sized memory operations by breaking them down into
+// simpler types which availability is described in the Backend. It also
+// provides a way to load and store sized chunks of memory (necessary for the
----------------
whose


================
Comment at: libc/src/string/memory_utils/sized_op.h:37
+  static constexpr size_t TYPE_SIZE = sizeof(type);
+  static_assert(Size >= TYPE_SIZE);
+  static constexpr size_t NEXT_SIZE = Size - TYPE_SIZE;
----------------
[nit] The mix of cases makes this difficult to read, please use `SIZE` (or better still, change the style guide for llvm-libc :) )


================
Comment at: libc/src/string/memory_utils/sized_op.h:39
+  static constexpr size_t NEXT_SIZE = Size - TYPE_SIZE;
+  using NextBlock = SizedOp<Backend, NEXT_SIZE>;
+
----------------
What about overlapping operations ?


================
Comment at: libc/src/string/memory_utils/sized_op.h:47
+
+  template <typename SrcAddrT> static inline auto loadType(SrcAddrT src) {
+    static_assert(IsAddressType<SrcAddrT>::Value && SrcAddrT::IS_READ);
----------------
The naming is confusing to me: `loadType` wants to read as "load the given type". Maybe `typedLoad`. or `loadFrom` ?


================
Comment at: libc/src/string/memory_utils/sized_op.h:47
+
+  template <typename SrcAddrT> static inline auto loadType(SrcAddrT src) {
+    static_assert(IsAddressType<SrcAddrT>::Value && SrcAddrT::IS_READ);
----------------
courbet wrote:
> The naming is confusing to me: `loadType` wants to read as "load the given type". Maybe `typedLoad`. or `loadFrom` ?
doc ?


================
Comment at: libc/src/string/memory_utils/sized_op.h:55
+  template <typename DstAddrT>
+  static inline void storeType(type value, DstAddrT dst) {
+    static_assert(IsAddressType<DstAddrT>::Value && DstAddrT::IS_WRITE);
----------------
ditto


================
Comment at: libc/src/string/memory_utils/sized_op.h:62
+
+  struct Value {
+    alignas(alignof(type)) ubyte payload[Size];
----------------
doc ?


================
Comment at: libc/src/string/memory_utils/sized_op.h:99
+  template <typename SrcAddrT1, typename SrcAddrT2>
+  static inline uint64_t different(SrcAddrT1 src1, SrcAddrT2 src2) {
+    const uint64_t current =
----------------
`is_different` or `differs` ? 


================
Comment at: libc/src/string/memory_utils/sized_op.h:102
+        Backend::template not_equals<type>(loadType(src1), loadType(src2));
+    if constexpr (NEXT_SIZE) {
+      return current | (NextBlock::different(offsetAddr<TYPE_SIZE>(src1),
----------------
` > 0`


================
Comment at: libc/src/string/memory_utils/sized_op.h:103
+    if constexpr (NEXT_SIZE) {
+      return current | (NextBlock::different(offsetAddr<TYPE_SIZE>(src1),
+                                             offsetAddr<TYPE_SIZE>(src2)));
----------------
The non-short-circuiting `|` warrants a comment. 


================
Comment at: libc/src/string/memory_utils/sized_op.h:116
+      return Backend::template three_way_cmp<type>(a, b);
+    // if (int32_t res = three_way_cmp<T>(a, b)) {
+    //   return res;
----------------
?


================
Comment at: libc/test/src/string/memory_utils/backend_test.cpp:13
+
+static char GetRandomChar() {
+  static constexpr const uint64_t a = 1103515245;
----------------
Looks like the ISO/IEC 9899 LCG seeded with `123456789`. Can you add a link ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126768