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

[Fangrui Song via Phabricator via libc-commits]

MaskRay added inline comments.


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72516