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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
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");
+}
----------------
gchatelet wrote:
> 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.
Actually I believe we still need to use `+`
See [[ https://github.com/llvm/llvm-project/blob/bf4d8f29524449c4114b457875cc6a8031c46092/clang/lib/Headers/intrin.h#L477 | clang implementation ]].

This is to inform the compiler that executing `rep mov` will change the underlying registers, it can't assume that `ECX`, `ESI`, `EDI` won't change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397