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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a subscriber: sivachandra.
gchatelet added inline comments.


================
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 {
----------------
courbet wrote:
> I don't see `Skip` being tested, or even used. Is it useful ?
It is currently used for aarch64 memset https://github.com/llvm/llvm-project/blob/main/libc/src/string/memory_utils/memset_implementations.h#L77-L104.
I've added tests.


================
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) {
----------------
courbet wrote:
> maybe: 
> 
> ```
> static_assert(SizedOp::SIZE >0);
> ```
> 
> to fail explicitly if `SizedOp` is not a statically sized operation.
By definition `SizedOp` **is** a statically sized operation 


================
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);
----------------
courbet wrote:
> 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.
I still don't fully understand the ordering consequences so I made it clear by adding a comment. Thx for the suggestion.


================
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) {
----------------
courbet wrote:
> Does this one even make sense if `runtime_size > 2*SizedOp::SIZE` ? Maybe add an assert ?
It does not make sense but I think we cannot use `assert` inside `memcpy` and other low level functions as `assert` could use them under the hood.
+ @sivachandra for confirmation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128335