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

[Kazu Hirata via Phabricator via llvm-commits]

kazu marked 5 inline comments as done.
kazu added a comment.

Please take a look.  Thanks!



================
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);
----------------
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`.


================
Comment at: llvm/include/llvm/ADT/bit.h:46
+// Declare these intrinsics manually rather including intrin.h. It's very
+// expensive, and MathExtras.h is popular.
+// #include <intrin.h>
----------------
RKSimon wrote:
> MathExtras.h ?
I've updated the comment.


================
Comment at: llvm/include/llvm/Support/MathExtras.h:23
 #include <limits>
 #include <type_traits>
 
----------------
RKSimon wrote:
> Can you confirm are all of these still needed?
include-what-you-use tells me that I can remove:

```
#include "llvm/Support/Compiler.h"
```

at least on a x86_64 Linux machine.

We still need all the others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142078