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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/CMakeLists.txt:78
+  compile_memcpy_with_flags(avx512 FLAGS -mavx512f)
+endif()
+
----------------
abrachet wrote:
> sivachandra wrote:
> > gchatelet wrote:
> > > This now creates the following memcpy implementations
> > >  - `__llvm_libc::memcpy_x86_64_avx512`
> > >  - `__llvm_libc::memcpy_x86_64_avx`
> > >  - `__llvm_libc::memcpy_x86_64_sse2`
> > >  - `__llvm_libc::memcpy_x86_64_sse`
> > >  - `__llvm_libc::memcpy_x86_64_unopt`
> > > 
> > > For shared libc, we need an ifunc like trampoline to select the correct version.
> > > For static libc, we need to select an implementation
> > > 
> > > @sivachandra how do you see this kind if code generation integrate into the more general cmake functions from `libc/cmake/modules/LLVMLibCRules.cmake`?
> > > I expect other memory functions to follow the same scheme.
> > Instead of building all the possible implementations, could we use the CMake [[ https://cmake.org/cmake/help/v3.14/command/try_compile.html | `try_compile` ]] and/or [[ https://cmake.org/cmake/help/v3.14/command/try_run.html | `try_run` ]] command to sniff out the best flags to use? I think `try_run` is more appropriate as I expect that we need to run the `cpuid` instruction? Also, compilers have a convenience macro `__cpuid` to run this instruction on x86/x86_64?
> > 
> > BTW, one can have ifuncs in static libraries as well. But, I do understand we want to avoid the overhead of the added indirection, so sniffing out at configure time is the best. If we can setup something for configure time sniffing, I believe we should be able use it (may be with straightforward extension/modification iff required) to use as the ifunc selector as well.
> I think this is going to get quickly outside of the scope of this patch. It would probably be better to work on this in a separate patch.
If there are any infrastructure pieces required to build a full solution, then we can do them separately as a prerequisite. However, if correct compile options are critical to memcpy implementation, then any code added to deduce them should belong to this patch. I agree that the patch will start to become too big. But, I think the additions to memcpy_utils and their tests can be split out to another prerequisite patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74397