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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Thanks @kazu!

Please can you add test coverage to ADT/BitTest.cpp ? By the looks of it we never added any equivalent to Support/MathExtrasTest.cpp :(



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


================
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>
----------------
MathExtras.h ?


================
Comment at: llvm/include/llvm/Support/MathExtras.h:23
 #include <limits>
 #include <type_traits>
 
----------------
Can you confirm are all of these still needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142078