[libc-commits] [PATCH] D128335: [libc][mem*] Introduce Algorithms for new mem framework

[Clement Courbet via Phabricator via libc-commits]

courbet added a comment.

First comments: statically sized operations.



================
Comment at: libc/src/string/memory_utils/algorithm.h:17
+// - Align
+//===----------------------------------------------------------------------===//
+
----------------
`// See each class for documentation.`


================
Comment at: libc/src/string/memory_utils/algorithm.h:30
+// An operation that allows to skip the specified amount of bytes.
+template <ptrdiff_t Bytes> struct Skip {
+  template <typename NextT> struct Then {
----------------
I don't see `Skip` being tested, or even used. Is it useful ?


================
Comment at: libc/src/string/memory_utils/algorithm.h:96
+    return SizedOp::isDifferent(tailAddr<SIZE>(src1, runtime_size),
+                              tailAddr<SIZE>(src2, runtime_size));
+  }
----------------
Please format the patch.


================
Comment at: libc/src/string/memory_utils/algorithm.h:107
+
+// Perform the operation on the first and last bytes of the buffer.
+// This is useful for overlapping operations.
----------------
I would say "the first and the last `SizedOp::Size` bytes of the buffer". That way it's clear that the `SizedOp` has to be a statically sized operation.


================
Comment at: libc/src/string/memory_utils/algorithm.h:118
+template <typename SizedOp> struct HeadTail {
+  template <typename DstAddrT, typename SrcAddrT>
+  static inline void copy(DstAddrT dst, SrcAddrT src, size_t runtime_size) {
----------------
maybe: 

```
static_assert(SizedOp::SIZE >0);
```

to fail explicitly if `SizedOp` is not a statically sized operation.


================
Comment at: libc/src/string/memory_utils/algorithm.h:127
+    static constexpr size_t SIZE = SizedOp::SIZE;
+    auto head = SizedOp::load(src);
+    auto tail = SizedOp::load(tailAddr<SIZE>(src, runtime_size));
----------------
`//Loads and stores cannot be interleaved because they might alias.`


================
Comment at: libc/src/string/memory_utils/algorithm.h:129
+    auto tail = SizedOp::load(tailAddr<SIZE>(src, runtime_size));
+    SizedOp::store(tailAddr<SIZE>(dst, runtime_size), tail);
+    SizedOp::store(dst, head);
----------------
Do you want to comment on the order of the stores ? Both are possible provided that they come after both loads. Is there a reason to chose one over the other ? Same for loads actually.


================
Comment at: libc/src/string/memory_utils/algorithm.h:142
+                                   size_t runtime_size) {
+    if (const auto res = SizedOp::isDifferent(src1, src2))
+      return res;
----------------
Can you comment on using short-circuiting/branching vs `|`.


================
Comment at: libc/src/string/memory_utils/algorithm.h:148
+  template <typename SrcAddrT1, typename SrcAddrT2>
+  static inline int32_t threeWayCmp(SrcAddrT1 src1, SrcAddrT2 src2,
+                                    size_t runtime_size) {
----------------
Does this one even make sense if `runtime_size > 2*SizedOp::SIZE` ? Maybe add an assert ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128335