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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/benchmarks/Memcmp.cpp:64
+    FunctionPrototype Function =
+        StringSwitch<FunctionPrototype>(FunctionName).Case("memcmp", &::memcmp);
+    return llvm::libc_benchmarks::benchmark(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72516