[libc-commits] [PATCH] D126768: [libc][mem*] Introduce Sized/Backends for new mem framework

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet marked 22 inline comments as done.
gchatelet added inline comments.


================
Comment at: libc/src/string/memory_utils/backend_aarch64.h:16-20
+  template <typename T, Temporality TS, Aligned AS,
+            cpp::EnableIfType<ScalarBackend::IsScalarType<T>, bool> = true>
+  static inline T load(const T *src) {
+    return ScalarBackend::template load<T, TS, AS>(src);
+  }
----------------
courbet wrote:
> It's not clear what this does: 
>  - if `T` scalar type, it delegates to the scalar backend, i.e. a noop.
>  - if not, the compiler will select the implementation in `ScalarBackend`, which will assert that `T` is a scalar type. 
> 
> What am I missing ?
For now we don't have any special types to handle for aarch64 so I've removed this backend.


================
Comment at: libc/src/string/memory_utils/backend_aarch64.h:23
+
+// Implementation of the SizedOp abstraction for the set operation.
+struct Zva64 {
----------------
courbet wrote:
> fix comment ?
It really is an implementation of the SizedOp abstraction but it only provides the set method.
I've updated the comment, I hope it's clearer now.


================
Comment at: libc/src/string/memory_utils/backend_scalar.h:14-15
+  static constexpr bool IsScalarType =
+      cpp::IsSameV<T, uint8_t> || cpp::IsSameV<T, uint16_t> ||
+      cpp::IsSameV<T, uint32_t> || cpp::IsSameV<T, uint64_t>;
+
----------------
courbet wrote:
> is that true of all architectures ?
As discussed, I've renamed the backend so it's explicit we only consider 64bit implementation for now.


================
Comment at: libc/src/string/memory_utils/backend_scalar.h:35
+    static_assert(IsScalarType<T>);
+    return (T(~0) / T(0xFF)) * T(value);
+  }
----------------
courbet wrote:
> It's not immediately obvious that this works for `uint64_t`. What about `~T(0) / T(0xFF)` ?
As discussed offline, let's go with `return (T(~0ULL) / T(0xFF)) * T(value);` since the ~ operator would promote `uint8_t` and `uint16_t` to `int`.


================
Comment at: libc/src/string/memory_utils/backend_x86.h:127
+inline uint64_t X86Backend::not_equals<__m128i>(__m128i a, __m128i b) {
+  using T = char __attribute__((__vector_size__(16)));
+  return _mm_movemask_epi8(T(a) != T(b));
----------------
courbet wrote:
> why not `_mm_cmpeq_epi8` instead ?
I would need `_mm_cmpneq_epi8` and it's cumbersome to express with Intel intrinsics. The instruction exists for `_mm512` though.


================
Comment at: libc/src/string/memory_utils/backend_x86.h:148
+inline uint64_t X86Backend::not_equals<__m256i>(__m256i a, __m256i b) {
+  using T = char __attribute__((__vector_size__(32)));
+  return _mm256_movemask_epi8(T(a) != T(b));
----------------
courbet wrote:
> why not `_mm256_cmpeq_epi8` instead ?
ditto


================
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>;
+
----------------
courbet wrote:
> What about overlapping operations ?
That's another design point that I've tested previously and that doesn't bring any benefits for fixed size operations so I kept the simpler design.

The generic algorithms are usually considering fixed-sized operations up to 3 or 4 and then rely on dynamically overlapping operations to cover sizes from N to 2xN.

So this design point only has a concrete impact for size 3.


================
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:
> courbet wrote:
> > The naming is confusing to me: `loadType` wants to read as "load the given type". Maybe `typedLoad`. or `loadFrom` ?
> doc ?
I went with `nativeLoad`/`nativeStore` as discussed offline


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126768