[LLVM][Support][NFC] Make multiple bitwise MathExtras functions constexpr

[gAlfonso-bit]

Summary

Submitted as an upstream fix

MathExtras has many useful tools. However, they aren't available to use in constexpr equations or functions. Originally this was a patch for swift llvm but it works upstream and as such I have put this PR upstream.

[gAlfonso-bit]

Made even more functions constexpr

[gAlfonso-bit]

Fix formatting

[gAlfonso-bit]

Fix compilation errors

[craig.topper]

Please upload patches with full context using -U999999 per the instructions here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[gAlfonso-bit]

In D107240#2926914, @craig.topper wrote:

Please upload patches with full context using -U999999 per the instructions here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Alright, I did.

[craig.topper]

llvm/include/llvm/Support/MathExtras.h

192

Does _BitScanReverse work in a constexpr in MSVC? The implementation in clang-cl doesn't work in constexpr, but clang-cl would use __builtin_clz.

[gAlfonso-bit]

llvm/include/llvm/Support/MathExtras.h

192

Yes it does. This is one of those exceptions where you can pass a reference because you don't read Index, but only write to it, and Index is in a constexpr anyway

[craig.topper]

llvm/include/llvm/Support/MathExtras.h

192

I tried it on godbolt and it didn't seem to work. Am I doing something wrong? https://godbolt.org/z/MnEY763E4

[gAlfonso-bit]

llvm/include/llvm/Support/MathExtras.h

192

Not sure, but so far LLVM compiles on Windows without a hitch, and the testbot is not having issues either

[gAlfonso-bit]

clang-tidy is complaining about function names, but changing every function instance would modify too many files

[craig.topper]

llvm/include/llvm/Support/MathExtras.h

192

Maybe that just means llvm never uses it in a context where it needs to be evaluated as a constexpr since your patch didn't add any new usages. So it might fail if someone tries to use the constexpr capability?

[gAlfonso-bit]

llvm/include/llvm/Support/MathExtras.h

192

Well, the other functions I made constexpr calls them, so that isn't it.

[gAlfonso-bit]

@craig.topper Only clang-tidy is having issues

[craig.topper]

I copied a chunk of this patch to godbolt and added two different constexpr usages of countLeadingZeros that both fail with MSVC https://godbolt.org/z/Maen4bac4 I'm guessing that because everything is a template, MSVC doesn't diagnose anything until you try to use it in constexpr context.

[craig.topper]

llvm/include/llvm/Support/MathExtras.h

116

Doesn't this mean that users of this that want to use it in constexpr can't compile that code with MSVC and would need a different solution just for MSVC? Is that useful for the project?

[gAlfonso-bit]

llvm/include/llvm/Support/MathExtras.h

116

No, since people who want to use it with constexpr but can't already exist, but this group gets smaller. Hopefully Microsoft finds a way, but there are downstream projects that would benefit from this already, like Swift.

[gAlfonso-bit]

By the way, it is not windows that gives clang tidy errors, but Linux.

[gAlfonso-bit]

Unfortunately it can't be constexpr for Windows at this time.

[gAlfonso-bit]

No build errors, only clang-tidy ones!

[gAlfonso-bit]

Remove contexpr from functions that call _BitScan Microsoft functions

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

llvm/include/llvm/Support/MathExtras.h

192

So I think I figured it out. Since I made the FIRST count function constexpr where the second value in the <> braces isn't constant, the compiler is using that instead of the non constexpr ones in these cases.

[craig.topper]

Is it possible to add unittests showing the count functions being used in constexpr contexts to prove that this will work with MSVC?

[gAlfonso-bit]

Unfortunately, as long as we are using c++14, there is actually no easy solution to this

[gAlfonso-bit]

@craig.topper I could use the libcpp_clz, which is constexpr, but that seems to be c++ 20 only.

[gAlfonso-bit]

I don't see how to progress here, so I am closing this for now

llvm/include/llvm/Support/MathExtras.h

 
 namespace detail {
 template <typename T, std::size_t SizeOfT> struct TrailingZerosCounter {
-  static unsigned count(T Val, ZeroBehavior) {
+  static constexpr unsigned count(T Val, ZeroBehavior) {
     if (!Val)
       return std::numeric_limits<T>::digits;
     if (Val & 0x1)

 
 #if defined(__GNUC__) || defined(_MSC_VER)
 template <typename T> struct TrailingZerosCounter<T, 4> {
-  static unsigned count(T Val, ZeroBehavior ZB) {
+  static constexpr unsigned count(T Val, ZeroBehavior ZB) {
     if (ZB != ZB_Undefined && Val == 0)
       return 32;
 

 
 #if !defined(_MSC_VER) || defined(_M_X64)
 template <typename T> struct TrailingZerosCounter<T, 8> {
-  static unsigned count(T Val, ZeroBehavior ZB) {
+  static constexpr unsigned count(T Val, ZeroBehavior ZB) {
     if (ZB != ZB_Undefined && Val == 0)
       return 64;
 

 /// \param ZB the behavior on an input of 0. Only ZB_Width and ZB_Undefined are
 ///   valid arguments.
 template <typename T>
-unsigned countTrailingZeros(T Val, ZeroBehavior ZB = ZB_Width) {
+constexpr unsigned countTrailingZeros(T Val, ZeroBehavior ZB = ZB_Width) {
   static_assert(std::numeric_limits<T>::is_integer &&
                     !std::numeric_limits<T>::is_signed,
                 "Only unsigned integral types are allowed.");

 
 namespace detail {
 template <typename T, std::size_t SizeOfT> struct LeadingZerosCounter {
-  static unsigned count(T Val, ZeroBehavior) {
+  static constexpr unsigned count(T Val, ZeroBehavior) {
     if (!Val)
       return std::numeric_limits<T>::digits;
 

 
 #if defined(__GNUC__) || defined(_MSC_VER)
 template <typename T> struct LeadingZerosCounter<T, 4> {
-  static unsigned count(T Val, ZeroBehavior ZB) {
+  static constexpr unsigned count(T Val, ZeroBehavior ZB) {
     if (ZB != ZB_Undefined && Val == 0)
       return 32;
 

 
 #if !defined(_MSC_VER) || defined(_M_X64)
 template <typename T> struct LeadingZerosCounter<T, 8> {
-  static unsigned count(T Val, ZeroBehavior ZB) {
+  static constexpr unsigned count(T Val, ZeroBehavior ZB) {
     if (ZB != ZB_Undefined && Val == 0)
       return 64;
 

 /// \param ZB the behavior on an input of 0. Only ZB_Width and ZB_Undefined are
 ///   valid arguments.
 template <typename T>
-unsigned countLeadingZeros(T Val, ZeroBehavior ZB = ZB_Width) {
+constexpr unsigned countLeadingZeros(T Val, ZeroBehavior ZB = ZB_Width) {
   static_assert(std::numeric_limits<T>::is_integer &&
                     !std::numeric_limits<T>::is_signed,
                 "Only unsigned integral types are allowed.");

 ///
 /// \param ZB the behavior on an input of 0. Only ZB_Max and ZB_Undefined are
 ///   valid arguments.
-template <typename T> T findFirstSet(T Val, ZeroBehavior ZB = ZB_Max) {
+template <typename T>
+constexpr T findFirstSet(T Val, ZeroBehavior ZB = ZB_Max) {
   if (ZB == ZB_Max && Val == 0)
     return std::numeric_limits<T>::max();
 

 ///
 /// \param ZB the behavior on an input of 0. Only ZB_Max and ZB_Undefined are
 ///   valid arguments.
-template <typename T> T findLastSet(T Val, ZeroBehavior ZB = ZB_Max) {
+template <typename T> constexpr T findLastSet(T Val, ZeroBehavior ZB = ZB_Max) {
   if (ZB == ZB_Max && Val == 0)
     return std::numeric_limits<T>::max();
 

 #define R2(n) n, n + 2 * 64, n + 1 * 64, n + 3 * 64
 #define R4(n) R2(n), R2(n + 2 * 16), R2(n + 1 * 16), R2(n + 3 * 16)
 #define R6(n) R4(n), R4(n + 2 * 4), R4(n + 1 * 4), R4(n + 3 * 4)
     R6(0), R6(2), R6(1), R6(3)
 #undef R2
 #undef R4
 #undef R6
 };
 
 /// Reverse the bits in \p Val.
-template <typename T>
-T reverseBits(T Val) {
+template <typename T> T reverseBits(T Val) {
   unsigned char in[sizeof(Val)];
   unsigned char out[sizeof(Val)];
   std::memcpy(in, &Val, sizeof(Val));

 }
 
 #if __has_builtin(__builtin_bitreverse8)
-template<>
-inline uint8_t reverseBits<uint8_t>(uint8_t Val) {
+template <> inline uint8_t reverseBits<uint8_t>(uint8_t Val) {
   return __builtin_bitreverse8(Val);
 }
 #endif
 
 #if __has_builtin(__builtin_bitreverse16)
-template<>
-inline uint16_t reverseBits<uint16_t>(uint16_t Val) {
+template <> inline uint16_t reverseBits<uint16_t>(uint16_t Val) {
   return __builtin_bitreverse16(Val);
 }
 #endif
 
 #if __has_builtin(__builtin_bitreverse32)
-template<>
-inline uint32_t reverseBits<uint32_t>(uint32_t Val) {
+template <> inline uint32_t reverseBits<uint32_t>(uint32_t Val) {
   return __builtin_bitreverse32(Val);
 }
 #endif
 
 #if __has_builtin(__builtin_bitreverse64)
-template<>
-inline uint64_t reverseBits<uint64_t>(uint64_t Val) {
+template <> inline uint64_t reverseBits<uint64_t>(uint64_t Val) {
   return __builtin_bitreverse64(Val);
 }
 #endif

 
 /// Make a 64-bit integer from a high / low pair of 32-bit integers.
 constexpr inline uint64_t Make_64(uint32_t High, uint32_t Low) {
-  return ((uint64_t)High << 32) | (uint64_t)Low;
+  return (static_cast<uint64_t>(High) << 32) | static_cast<uint64_t>(Low);
 }
 
 /// Checks if an integer fits into the given bit width.
 template <unsigned N> constexpr inline bool isInt(int64_t x) {
-  return N >= 64 || (-(INT64_C(1)<<(N-1)) <= x && x < (INT64_C(1)<<(N-1)));
+  return N >= 64 ||
+         (-(INT64_C(1) << (N - 1)) <= x && x < (INT64_C(1) << (N - 1)));
 }
 // Template specializations to get better code for common cases.
 template <> constexpr inline bool isInt<8>(int64_t x) {

 }
 
 /// Checks if an unsigned integer fits into the given (dynamic) bit width.
-inline bool isUIntN(unsigned N, uint64_t x) {
+constexpr inline bool isUIntN(unsigned N, uint64_t x) {
   return N >= 64 || x <= maxUIntN(N);
 }
 
 /// Checks if an signed integer fits into the given (dynamic) bit width.
-inline bool isIntN(unsigned N, int64_t x) {
+constexpr inline bool isIntN(unsigned N, int64_t x) {
   return N >= 64 || (minIntN(N) <= x && x <= maxIntN(N));
 }
 

 /// \param ZB the behavior on an input of all ones. Only ZB_Width and
 /// ZB_Undefined are valid arguments.
 template <typename T>
-unsigned countLeadingOnes(T Value, ZeroBehavior ZB = ZB_Width) {
+constexpr unsigned countLeadingOnes(T Value, ZeroBehavior ZB = ZB_Width) {
   static_assert(std::numeric_limits<T>::is_integer &&
                     !std::numeric_limits<T>::is_signed,
                 "Only unsigned integral types are allowed.");

 /// \param ZB the behavior on an input of all ones. Only ZB_Width and
 /// ZB_Undefined are valid arguments.
 template <typename T>
-unsigned countTrailingOnes(T Value, ZeroBehavior ZB = ZB_Width) {
+constexpr unsigned countTrailingOnes(T Value, ZeroBehavior ZB = ZB_Width) {
   static_assert(std::numeric_limits<T>::is_integer &&
                     !std::numeric_limits<T>::is_signed,
                 "Only unsigned integral types are allowed.");

 
 namespace detail {
 template <typename T, std::size_t SizeOfT> struct PopulationCounter {
-  static unsigned count(T Value) {
+  static constexpr unsigned count(T Value) {
     // Generic version, forward to 32 bits.
     static_assert(SizeOfT <= 4, "Not implemented!");
 #if defined(__GNUC__)

 };
 
 template <typename T> struct PopulationCounter<T, 8> {
-  static unsigned count(T Value) {
+  static constexpr unsigned count(T Value) {
 #if defined(__GNUC__)
     return __builtin_popcountll(Value);
 #else

 /// Count the number of set bits in a value.
 /// Ex. countPopulation(0xF000F000) = 8
 /// Returns 0 if the word is zero.
-template <typename T>
-inline unsigned countPopulation(T Value) {
+template <typename T> constexpr inline unsigned countPopulation(T Value) {
   static_assert(std::numeric_limits<T>::is_integer &&
                     !std::numeric_limits<T>::is_signed,
                 "Only unsigned integral types are allowed.");

 /// Return the floor log base 2 of the specified value, -1 if the value is zero.
 /// (32 bit edition.)
 /// Ex. Log2_32(32) == 5, Log2_32(1) == 0, Log2_32(0) == -1, Log2_32(6) == 2
-inline unsigned Log2_32(uint32_t Value) {
+constexpr inline unsigned Log2_32(uint32_t Value) {
   return 31 - countLeadingZeros(Value);
 }
 
 /// Return the floor log base 2 of the specified value, -1 if the value is zero.
 /// (64 bit edition.)
-inline unsigned Log2_64(uint64_t Value) {
+constexpr inline unsigned Log2_64(uint64_t Value) {
   return 63 - countLeadingZeros(Value);
 }
 
 /// Return the ceil log base 2 of the specified value, 32 if the value is zero.
 /// (32 bit edition).
 /// Ex. Log2_32_Ceil(32) == 5, Log2_32_Ceil(1) == 0, Log2_32_Ceil(6) == 3
-inline unsigned Log2_32_Ceil(uint32_t Value) {
+constexpr inline unsigned Log2_32_Ceil(uint32_t Value) {
   return 32 - countLeadingZeros(Value - 1);
 }
 
 /// Return the ceil log base 2 of the specified value, 64 if the value is zero.
 /// (64 bit edition.)
-inline unsigned Log2_64_Ceil(uint64_t Value) {
+constexpr inline unsigned Log2_64_Ceil(uint64_t Value) {
   return 64 - countLeadingZeros(Value - 1);
 }
 
 /// Return the greatest common divisor of the values using Euclid's algorithm.
-template <typename T>
-inline T greatestCommonDivisor(T A, T B) {
+template <typename T> constexpr inline T greatestCommonDivisor(T A, T B) {
   while (B) {
     T Tmp = B;
     B = A % B;

   return A;
 }
 
-inline uint64_t GreatestCommonDivisor64(uint64_t A, uint64_t B) {
+constexpr inline uint64_t GreatestCommonDivisor64(uint64_t A, uint64_t B) {
   return greatestCommonDivisor<uint64_t>(A, B);
 }
 

 
 /// Returns the next power of two (in 64-bits) that is strictly greater than A.
 /// Returns zero on overflow.
-inline uint64_t NextPowerOf2(uint64_t A) {
+constexpr inline uint64_t NextPowerOf2(uint64_t A) {
   A |= (A >> 1);
   A |= (A >> 2);
   A |= (A >> 4);

 
 /// Returns the power of two which is less than or equal to the given value.
 /// Essentially, it is a floor operation across the domain of powers of two.
-inline uint64_t PowerOf2Floor(uint64_t A) {
-  if (!A) return 0;
+constexpr inline uint64_t PowerOf2Floor(uint64_t A) {
+  if (!A)
+    return 0;
   return 1ull << (63 - countLeadingZeros(A, ZB_Undefined));
 }
 
 /// Returns the power of two which is greater than or equal to the given value.
 /// Essentially, it is a ceil operation across the domain of powers of two.
-inline uint64_t PowerOf2Ceil(uint64_t A) {
+constexpr inline uint64_t PowerOf2Ceil(uint64_t A) {
   if (!A)
     return 0;
   return NextPowerOf2(A - 1);

 }
 
 /// Returns the integer nearest(Numerator / Denominator).
-inline uint64_t divideNearest(uint64_t Numerator, uint64_t Denominator) {
+constexpr inline uint64_t divideNearest(uint64_t Numerator,
+                                        uint64_t Denominator) {
   return (Numerator + (Denominator / 2)) / Denominator;
 }
 

 /// Subtract two unsigned integers, X and Y, of type T and return the absolute
 /// value of the result.
 template <typename T>
-std::enable_if_t<std::is_unsigned<T>::value, T> AbsoluteDifference(T X, T Y) {
+constexpr std::enable_if_t<std::is_unsigned<T>::value, T>
+AbsoluteDifference(T X, T Y) {
   return X > Y ? (X - Y) : (Y - X);
 }
 

 /// Use this rather than HUGE_VALF; the latter causes warnings on MSVC.
 extern const float huge_valf;
 
-
 /// Add two signed integers, computing the two's complement truncated result,
 /// returning true if overflow occured.
 template <typename T>

     return UX > (static_cast<U>(std::numeric_limits<T>::max())) / UY;
 }
 
-} // End llvm namespace
+} // namespace llvm
 
 #endif

llvm/lib/Support/MathExtras.cpp

 #ifdef _MSC_VER
 #include <limits>
 #else
-#include <math.h>
+#include <cmath>
 #endif
 
 namespace llvm {
 
 #if defined(_MSC_VER)
-  // Visual Studio defines the HUGE_VAL class of macros using purposeful
-  // constant arithmetic overflow, which it then warns on when encountered.
-  const float huge_valf = std::numeric_limits<float>::infinity();
+constexpr float huge_valf = std::numeric_limits<float>::infinity();
 #else
-  const float huge_valf = HUGE_VALF;
+constexpr float huge_valf = HUGE_VALF;
 #endif
 
 } // namespace llvm