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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
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);
----------------
kazu wrote:
> dblaikie wrote:
> > 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?
> Hmm.  I am not sure if I want to go as far as requiring users to have LTO.
> 
> I think what to do with these forward declarations is somewhat orthogonal to moving these bit counting functions to `bit.h`.
Oh, sorry, fair enough - if it's just moving things around they might be unfortunate but it's no worse and not bad enough to relitigate. Sorry for the distraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142078