This is an archive of the discontinued LLVM Phabricator instance.

[libc][mem*] Introduce Sized/Backends for new mem framework
ClosedPublic

Authored by gchatelet on Jun 1 2022, 4:02 AM.

Details

Summary

This patch is a subpart of D125768 intented to make the review easier.

The SizedOp struct represents operations to be performed on a certain number of bytes.
It is responsible for breaking them down into platform types and forwarded to the Backend.

The Backend struct represents a lower level abstraction that works only on types (uint8_t, __m128i, ...).
It is similar to instruction selection.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 1 2022, 4:02 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 1 2022, 4:02 AM
gchatelet requested review of this revision.Jun 1 2022, 4:02 AM

As a very high level, design comment, I would like to note that I think we should strive to get into a state where the lowering of a SizedOp is done via a instrinsic call for all operation type (right now, this is only the case for memcpy).

The idea is that there are currently two places where we know how to lower a memory operation: the Backend, and the compiler (through the __builtin_memxxx_inline). In the future, any improvement to the Backend will only impact the codegen for memxxx, but not the expansion of a memxxx(p, q, constant_size). One way to ensure non-divergence between the Backend implementations and the builtins is to actually use the builtins for lowering (at least in the "normal", non-temporal case).

So eventually, all implementations should look like the one for copy:

 if constexpr (LLVM_LIBC_USE_BUILTIN_MEMXXX_INLINE && /*is normal*/) {
     // delegate optimized operation to compiler.
     __builtin_memxxx_inline(dst.ptr(), src.ptr(), Size);
     return;
   }
   // Fancy c++ implementation.
}

This is especially true for memcmp (I'm not sure how much love the ExpandMemcmp pass has had wrt aarch64), and memset (for example, __builtin_memset does not currently know to use zva: https://godbolt.org/z/6noee3Y6c).

courbet added inline comments.Jun 1 2022, 7:01 AM
libc/src/string/memory_utils/backend_aarch64.h
17–21

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 ?

24

fix comment ?

libc/src/string/memory_utils/backend_scalar.h
15–16

is that true of all architectures ?

22

is that true of all architectures ?

36

It's not immediately obvious that this works for uint64_t. What about ~T(0) / T(0xFF) ?

gchatelet updated this revision to Diff 433704.Jun 2 2022, 2:44 AM
gchatelet marked 5 inline comments as done.
  • Address comments
courbet added inline comments.Jun 2 2022, 5:32 AM
libc/src/string/memory_utils/backend_x86.h
44

"unspecialized case". And the function should have a comment saying something like:

// This function is specialized below.

And specialize it for non-temporal loads if you want a better user message.

57

ditto

60

// Forwarding to base class because c++ does not allow specializing base class methods.

70

I've just noticed that all functions in this patch should be using threeWayCmp case...

82

doc?

128

why not _mm_cmpeq_epi8 instead ?

149

why not _mm256_cmpeq_epi8 instead ?

courbet added inline comments.Jun 3 2022, 5:43 AM
libc/src/string/memory_utils/backend_x86.h
1

Missing header (same above).

libc/src/string/memory_utils/backends.h
12

"native loads and stores", right ?

36

[Nit]sentences.

libc/src/string/memory_utils/sized_op.h
11

whose

37

[nit] The mix of cases makes this difficult to read, please use SIZE (or better still, change the style guide for llvm-libc :) )

39

What about overlapping operations ?

47

The naming is confusing to me: loadType wants to read as "load the given type". Maybe typedLoad. or loadFrom ?

47

doc ?

55

ditto

62

doc ?

99

is_different or differs ?

102

> 0

103

The non-short-circuiting | warrants a comment.

116

?

libc/test/src/string/memory_utils/backend_test.cpp
13

Looks like the ISO/IEC 9899 LCG seeded with 123456789. Can you add a link ?

Matt added a subscriber: Matt.Jun 13 2022, 1:00 PM
gchatelet marked 22 inline comments as done.Jun 15 2022, 5:10 AM
gchatelet added inline comments.
libc/src/string/memory_utils/backend_aarch64.h
17–21

For now we don't have any special types to handle for aarch64 so I've removed this backend.

24

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.

libc/src/string/memory_utils/backend_scalar.h
15–16

As discussed, I've renamed the backend so it's explicit we only consider 64bit implementation for now.

36

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.

libc/src/string/memory_utils/backend_x86.h
128

I would need _mm_cmpneq_epi8 and it's cumbersome to express with Intel intrinsics. The instruction exists for _mm512 though.

149

ditto

libc/src/string/memory_utils/sized_op.h
39

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.

47

I went with nativeLoad/nativeStore as discussed offline

libc/test/src/string/memory_utils/backend_test.cpp
13

Added documentation and used the C++ "recommended" settings.

gchatelet updated this revision to Diff 437115.Jun 15 2022, 5:11 AM
gchatelet marked 5 inline comments as done.
  • Address comments
gchatelet updated this revision to Diff 437117.Jun 15 2022, 5:29 AM
  • Rename backend
courbet accepted this revision.Jun 15 2022, 6:30 AM
courbet added inline comments.
libc/src/string/memory_utils/sized_op.h
138

?

libc/test/src/string/memory_utils/backend_test.cpp
2

Header ?

This revision is now accepted and ready to land.Jun 15 2022, 6:30 AM
gchatelet updated this revision to Diff 438970.Jun 22 2022, 4:19 AM
gchatelet marked 2 inline comments as done.
  • Address comments
This revision was landed with ongoing or failed builds.Jun 22 2022, 4:21 AM
This revision was automatically updated to reflect the committed changes.