[libc-commits] [PATCH] D74397: [libc] Adding memcpy implementation for x86_64

Wed Feb 12 04:59:47 PST 2020

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

================
Comment at: libc/src/string/memcpy_x86_64.cpp:12
+#include "src/string/memory_utils/memcpy_utils.h"
+
+namespace __llvm_libc {
----------------
abrachet wrote:
> It might be worth it as a sanity check because this is an x86 specific file to static_assert LLVM_LIBC_CACHELINE_SIZE == 64
Agreed but I'm deferring the addition of the static assert to the point where we know which layout to use for the source code. I'm keeping your comment as `Not Done` so I don't forget to add it later.

================
Comment at: libc/src/string/memcpy_x86_64.cpp:15
+
+static void CopyRepMovsb(char *dst, const char *src, size_t count) {
+  LIBC_INLINE_ASM("rep movsb" : "+c"(count), "+S"(src), "+D"(dst) : : "memory");
----------------
sivachandra wrote:
> Is this the only reason this file name has the `_86_64` in it? If yes, is this function the only machine specific piece for other archs as well? If yes, we should put it in a header file and use the same scheme we use for `LLVM_LIBC_CACHELINE_SIZE`.
Yes for now. I'd yet to see how this implementation performs on other architectures (ARM, PowerPC, ...). But yes indeed this implementation will at least be common to `x86` and `x86_64`. I'll adapt the code consequently,

================
Comment at: libc/src/string/memcpy_x86_64.cpp:16
+static void CopyRepMovsb(char *dst, const char *src, size_t count) {
+  LIBC_INLINE_ASM("rep movsb" : "+c"(count), "+S"(src), "+D"(dst) : : "memory");
+}
----------------
abrachet wrote:
> MaskRay wrote:
> > `LIBC_INLINE_ASM` does not improve readability.
> > 
> > `asm volatile`
> > 
> > Any reason using constraint modifier `+`? It adds two operands, one input and one output. Just use `"D"(dst), "S"(src), "c"(count)`
> +1 I think the reasoning behind the macro is for MSVC, which as far as I can tell doesn't support GCC's syntax anyway https://docs.microsoft.com/en-us/cpp/assembler/inline/asm?view=vs-2019
@MaskRay 

`LIBC_INLINE_ASM` was available so I used it,
I thought this would be used to mask out the differences between the different compilers but I'm not so sure. The operands/constraints modifiers might be sufficiently different that it is not possible to write it in a compiler agnostic way.

For instance `asm` is not to be used with msv (from the link @abrachet sent).

As for the `+` modifier they are not needed in this context indeed.

================
Comment at: libc/src/string/memcpy_x86_64.cpp:21-30
+  if (count == 0)
+    return;
+  if (count == 1)
+    return Copy<1>(dst, src);
+  if (count == 2)
+    return Copy<2>(dst, src);
+  if (count == 3)
----------------
abrachet wrote:
> Off of intuition I would imagine these are very rare sizes to call `memcpy` with. Would it be better to do something like `if (count < 5) goto smallCount;` and move these down?
> It's worth linking while we are here @ckennelly's [[ http://lists.llvm.org/pipermail/libc-dev/2020-January/000032.html | thread ]] from a few weeks ago. "For `memcpy`, 96% of sizes are <= 128 bytes.  99% are <= 1024 bytes."
> Off of intuition I would imagine these are very rare sizes to call memcpy with

They are not rare actually.
The rationale here is to have a branching cost that is proportional to the copy cost.

Now since the code is C++ the compiler //sees// it and //understands// the semantic.
For people using Profile Guided Optimization techniques the compiler will be able to reorder the branches according to branching probabilities.

Thx for linking in the thread.

================
Comment at: libc/src/string/memcpy_x86_64.cpp:44
+  // else CopyAligned is used to to kRepMovsBSize and then RepMovsb.
+  constexpr size_t kRepMovsBSize = -1;
+  if (count <= kRepMovsBSize)
----------------
sivachandra wrote:
> abrachet wrote:
> > I'm not sure I'm understanding this. It's not like we can change this at compile time, line 47 is dead code.
> I guess this relates to the customization @gchatelet mentioned about in the patch description.
Yes as I stated in the introduction of the patch this is for early feedback. Ultimately `kRepMovsBSize` needs to be defined by the environment.

================
Comment at: libc/src/string/memcpy_x86_64.cpp:46
+  if (count <= kRepMovsBSize)
+    return CopyAligned<32>(dst, src, count);
+  CopyRepMovsb(dst, src, count);
----------------
abrachet wrote:
> Why was 32 chosen?
It gave good results : )
`CopyAligned<N>` is prone to memory reloads so there's a balance between copying big chunks and reloading data from memory.

To be honest I would like a few pieces of the code to be customizable (this is one) so we could be benchmarking the cross-product of the implementations and pick the best performing ones per uarch.

The overall design should be better documented for sure, I'll work on it.

Code size is especially important for icache pressure and as you saw the generated code is quite small.

================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:9
+
+#define LLVM_LIBC_MEMPCY_MONITOR monitor_memcpy
+#include "src/string/memory_utils/memcpy_utils.h"
----------------
sivachandra wrote:
> Where is `monitor_memcpy` defined?
Further down after the `GetTrace` function.

================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
sivachandra wrote:
> abrachet wrote:
> > D74091 Was also wanting to use `assert`/`abort` we should add them. Also that comment is funny to me because its the only thing in assert.h :)
> Point taken. I will prepare something to address this.
I think that would be nice to have the functions in `memcpy_utils.h` have some precondition checking, an `abort` function would be useful indeed,
```
#if not defined(NDEBUG)
// check precondition
#endif

```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397