[PATCH] D142078: [llvm] Move bit counting functions to bit.h (NFC)

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/bit.h:44-54
+#ifdef _MSC_VER
+// Declare these intrinsics manually rather including intrin.h. It's very
+// expensive, and MathExtras.h is popular.
+// #include <intrin.h>
+extern "C" {
+unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
+unsigned char _BitScanForward64(unsigned long *_Index, unsigned __int64 _Mask);
----------------
RKSimon wrote:
> kazu wrote:
> > dblaikie wrote:
> > > Instead of this, perhaps we could write small/simple wrappers, avoiding the need to forward declare things from platform headers? & possibly wrapping the basic operations/differences between the platforms in a more direct way, then exposing these higher level primitives based on those abstractions?
> > Do you mean something like this?
> > 
> > ```
> > namespace detail {
> > #if __has_builtin(__builtin_clz) || defined(__GNUC__)
> > inline constexpr bool has_clz = true;
> > inline unsigned clz(unsigned Val) {
> >   return __builtin_clz(Val)
> > }
> > #else defined(_MSC_VER)
> > inline constexpr bool has_clz = true;
> > inline unsigned clz(unsigned Val) {
> >   unsigned long Index;
> >   _BitScanForward(&Index, Val);
> >   return Index;
> > }
> > #else
> > inline constexpr bool has_clz = false;
> > #endif
> > }  // namespace detail
> > ```
> > 
> > Then implement `llvm::countl_zero` with `llvm::detail::has_clz` and `llvm::detail:clz`?
> > 
> > If so, I like this idea, but I don't see how we can avoid forward-declaring things.  The comment says `intrin.h` is very expensive although I don't know whether the comment is still applicable today or how old the comment is.
> intrin.h is still (or possibly more) expensive to include, so avoiding use in a common header like this would be best.
I was thinking of an out of line solution - where the use of `__builtin_clz` and `_BitScanForward` are in a .cpp file, not in a header. I guess then performance is so bad that LTO is required to get to something reasonable? Maybe that's OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142078