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

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/src/string/memory_utils/algorithm.h:129
+  static inline void move(DstAddrT dst, SrcAddrT src, size_t runtime_size) {
+    static constexpr size_t SIZE = SizedOp::SIZE;
+    // The load and store operations can be performed in any order as long as
----------------
I think this should be `BLOCK_SIZE` too.


================
Comment at: libc/src/string/memory_utils/algorithm.h:287
+
+// Aligns and calls the subsequent NextT operation
+//
----------------
"Aligns using a statically-sized `SizedOp` operation, then calls..."


================
Comment at: libc/src/string/memory_utils/algorithm.h:290
+// e.g. A 16-byte Destination Aligned 32-byte Loop Copy can be written as:
+// Align<16, Arg::Dst>::Then<Loop<32>>::copy(dst, src, runtime_size);
+enum class Arg { _1, _2, Dst = _1, Src = _2, Lhs = _1, Rhs = _2 };
----------------
`16` is not a `SizedOp`: this is not consistent with the code below.


================
Comment at: libc/src/string/memory_utils/algorithm.h:386
+private:
+  static constexpr size_t SIZE = SizedOp::SIZE;
+  static_assert(SIZE > 1);
----------------
I think this should be named `ALIGN_OP_SIZE` or something like this.


================
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) {
----------------
gchatelet wrote:
> 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 
That's what I want to check :) 

I think that you essentially have two kinds of operations:
 - What I was calling "statically sized operations": `_1`, `_2`, ..., ``.
 - 

Maybe a better name would be "Composite operations" for the later and "Elementary operations" for the former.

My point is that I think some operations only take elementary operations, for example, this does not really make sense:

`HeadTail<Loop<_16>>`

We might even want `static constexpr  bool IS_ELEMENTARY = true`, or something like this.

WDYT ?





================
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) {
----------------
sivachandra wrote:
> gchatelet wrote:
> > 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.
> Yes, for now lets avoid asserts. I will get back to you with a detailed answer in the coming weeks.
OK, can you at least add a comment ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128335