[libc-commits] [PATCH] D72516: [llvm-libc] Add memory function benchmarks

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a comment.

@ckennelly looks good to you?



================
Comment at: libc/utils/benchmarks/Memcmp.cpp:64
+    FunctionPrototype Function =
+        StringSwitch<FunctionPrototype>(FunctionName).Case("memcmp", &::memcmp);
+    return llvm::libc_benchmarks::benchmark(
----------------
sivachandra wrote:
> MaskRay wrote:
> > sivachandra wrote:
> > > MaskRay wrote:
> > > > abrachet wrote:
> > > > > sivachandra wrote:
> > > > > > ckennelly wrote:
> > > > > > > Should we setup an analogous benchmark for `bcmp`?
> > > > > > Shoud llvm-libc provide bcmp at all?
> > > > > In what I believe was @gchatelet's email on the list said they were hoping `!memcmp(...)` would be emitted as `!bcmp(...)` for efficiency. [[ https://godbolt.org/z/5zBm4k | clang ]] does make this optimization.
> > > > TargetLibraryInfo.cpp
> > > > 
> > > > ```
> > > > static bool hasBcmp(const Triple &TT) {
> > > >   // Posix removed support from bcmp() in 2001, but the glibc and several
> > > >   // implementations of the libc still have it.
> > > >   if (TT.isOSLinux())
> > > >     return TT.isGNUEnvironment() || TT.isMusl();
> > > >   // Both NetBSD and OpenBSD are planning to remove the function. Windows does
> > > >   // not have it.
> > > >   return TT.isOSFreeBSD() || TT.isOSSolaris();
> > > > }
> > > > ```
> > > > 
> > > > On certain platforms (including Linux), memcmp can be optimized to bcmp if preferable.
> > > To be honest, I do not have any opinion. If others more knowledgeable in this area decide llvm-libc should have a `bcmp`, then we will go with it.
> > > 
> > > Just pointing out though: musl's `bcmp` just calls into `memcmp`, and in glibc, `bcmp` is an alias of `memcmp`.
> > POSIX.1-2001 marked bcmp obsoleted and POSIX.1-2008 deleted bcmp, but in reality, bcmp has performance benefits (the 3-state -> 2-state transformation allows unordered comparison).
> > 
> > Eventually we should have bcmp. Before it is optimized, you may make it an alias of memcmp.
> To be clear: I understand why `bcmp` is/can be faster when doing a plain equality check. My point is specifically about why a libc should provide it, especially when it is being removed from the standards. And, as I said, if libc happens to be the best place for `bcmp`, then so be it. May be alternates like compiler intrinsics have been considered (I will be surprised if not.) So, when adding bcmp, we should ideally also add justification for why a libc is the right place.
Yes indeed we already have an optimization in clang to convert `(bool)memcmp` to `bcmp`, this is the second bullet in the [LLVM 9.0.0 release notes](http://releases.llvm.org/9.0.0/docs/ReleaseNotes.html#noteworthy-optimizations).

Since the current code is benchmarking the host's libc and not llvm-libc (yet) I will just provide a comment to make sure we benchmark `bcmp` once it is known to be available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72516