[libc][math] Improved FBits performance and readablity.

[orex]

Summary

Some function added in preparation to fmod commit.

[orex]

Removed common.h

[lntue]

libc/src/__support/FPUtil/FPBits.h

129

I forget whether for 80-bit long double, bits 80-127 might contain garbage or not. Would you mind double checking that? If that's the case, you might simply add EXPONENT_MANTISSA_MASK (or abbreviated to EXP_MANT_MASK is up to you) and use that instead of ~SIGN_MASK.

[orex]

libc/src/__support/FPUtil/FPBits.h

129

Probably, I did not get your idea fully. long double representation is significantly different from float, double or quad therefore it is covered by separate FBits class (see LongDoubleBits.h). Do you want me to improve that class also? If yes, I don't think that it should be done in the review. There are a lot of checks which should be done and I prefer it to be in a separate review.
As for the mask, it looks like that it is a good idea. I'll implement this.

[orex]

Added EXP_MANT_MASK.

[sivachandra]

As with the other patch, while this is accepted, please hold-off on submitting until further notice here.

libc/src/__support/FPUtil/FloatProperties.h

36

We should use a more formal tone. So, may be: "Exponent and mantissa masks are not as expected."

50

Why is multiplication by 8 preferred over shift by 3?

[orex]

Update static_assert message.

[orex]

libc/src/__support/FPUtil/FloatProperties.h

50

Just to improve readability. Everybody knows, that byte has 8 bits. But in previous representation it should:

1. Understand that << refers to explicit multiplication, but not bit/masks operations as everywhere else in the code.
2. Remember that 2^3=8.

Of course, it is not a big problem, but the code is still complex and need attention. And we should not add extra complications for nothing.

[sivachandra]

libc/src/__support/FPUtil/FloatProperties.h

50

To be absolutely clear, I am OK with using * 8 in this patch. At the same time, I would have found the reasoning more appealing if it was like, "we should let the compilers choose the best instructions for the semantic of multiplying by 8."

[orex]

libc/src/__support/FPUtil/FloatProperties.h

50

To be absolutely clear, I'll use your suggestion in the future. It is absolutely reasonable. But in this particular case, this is a` constexpr`, therefore the "best instruction" never appears in the code. It will be done during the compile time, so will not affect runtime at all.

[sivachandra]

Feel free to submit now.

libc/src/__support/FPUtil/FPBits.h

template <typename T> struct FPBits {
   void set_mantissa(UIntType mantVal) {
     mantVal &= (FloatProp::MANTISSA_MASK);
     bits &= ~(FloatProp::MANTISSA_MASK);
     bits |= mantVal;
   }
 
   UIntType get_mantissa() const { return bits & FloatProp::MANTISSA_MASK; }
 
+  // The function return mantissa with implicit bit set for normal values.
+  constexpr UIntType get_explicit_mantissa() {
+    return (FloatProp::MANTISSA_MASK + 1) | (FloatProp::MANTISSA_MASK & bits);
+  }
+
   void set_unbiased_exponent(UIntType expVal) {
     expVal = (expVal << (FloatProp::MANTISSA_WIDTH)) & FloatProp::EXPONENT_MASK;
     bits &= ~(FloatProp::EXPONENT_MASK);
     bits |= expVal;
   }
 
   uint16_t get_unbiased_exponent() const {
     return uint16_t((bits & FloatProp::EXPONENT_MASK) >>
                     (FloatProp::MANTISSA_WIDTH));
   }
 
   void set_sign(bool signVal) {
-    bits &= ~(FloatProp::SIGN_MASK);
-    UIntType sign = UIntType(signVal) << (FloatProp::BIT_WIDTH - 1);
-    bits |= sign;
+    bits |= FloatProp::SIGN_MASK;
+    if (!signVal)
+      bits -= FloatProp::SIGN_MASK;
   }
 
-  bool get_sign() const {
-    return ((bits & FloatProp::SIGN_MASK) >> (FloatProp::BIT_WIDTH - 1));
-  }
+  bool get_sign() const { return (bits & FloatProp::SIGN_MASK) != 0; }
 
   static_assert(sizeof(T) == sizeof(UIntType),
                 "Data type and integral representation have different sizes.");
 
   static constexpr int EXPONENT_BIAS = (1 << (ExponentWidth<T>::VALUE - 1)) - 1;
   static constexpr int MAX_EXPONENT = (1 << ExponentWidth<T>::VALUE) - 1;
 
   static constexpr UIntType MIN_SUBNORMAL = UIntType(1);
   static constexpr UIntType MAX_SUBNORMAL =
       (UIntType(1) << MantissaWidth<T>::VALUE) - 1;
   static constexpr UIntType MIN_NORMAL =
       (UIntType(1) << MantissaWidth<T>::VALUE);
   static constexpr UIntType MAX_NORMAL =
       ((UIntType(MAX_EXPONENT) - 1) << MantissaWidth<T>::VALUE) | MAX_SUBNORMAL;
 
-  // We don't want accidental type promotions/conversions so we require exact
+  // We don't want accidental type promotions/conversions, so we require exact
   // type match.
   template <typename XType,
             cpp::EnableIfType<cpp::IsSame<T, XType>::Value, int> = 0>
   constexpr explicit FPBits(XType x)
       : bits(__llvm_libc::bit_cast<UIntType>(x)) {}
 
   template <typename XType,
             cpp::EnableIfType<cpp::IsSame<XType, UIntType>::Value, int> = 0>
template <typename T> struct FPBits {
 
   UIntType uintval() const { return bits; }
 
   int get_exponent() const {
     return int(get_unbiased_exponent()) - EXPONENT_BIAS;
   }
 
   bool is_zero() const {
-    return get_mantissa() == 0 && get_unbiased_exponent() == 0;
+    // Remove sign bit by shift
+    return (bits << 1) == 0;
   }
 
   bool is_inf() const {
-    return get_mantissa() == 0 && get_unbiased_exponent() == MAX_EXPONENT;
+    return (bits & FloatProp::EXP_MANT_MASK) == FloatProp::EXPONENT_MASK;
   }
 
   bool is_nan() const {
-    return get_unbiased_exponent() == MAX_EXPONENT && get_mantissa() != 0;
+    return (bits & FloatProp::EXP_MANT_MASK) > FloatProp::EXPONENT_MASK;
   }
 
-  bool is_inf_or_nan() const { return get_unbiased_exponent() == MAX_EXPONENT; }
-
-  static FPBits<T> zero() { return FPBits(); }
+  bool is_inf_or_nan() const {
+    return (bits & FloatProp::EXPONENT_MASK) == FloatProp::EXPONENT_MASK;
+  }
 
-  static FPBits<T> neg_zero() {
-    return FPBits(UIntType(1) << (sizeof(UIntType) * 8 - 1));
+  static constexpr FPBits<T> zero(bool sign = false) {
+    return FPBits(sign ? FloatProp::SIGN_MASK : UIntType(0));
   }
 
-  static FPBits<T> inf() {
+  static constexpr FPBits<T> neg_zero() { return zero(true); }
+
+  static constexpr FPBits<T> inf() {
     FPBits<T> bits;
     bits.set_unbiased_exponent(MAX_EXPONENT);
     return bits;
   }
 
-  static FPBits<T> neg_inf() {
+  static constexpr FPBits<T> neg_inf() {
     FPBits<T> bits = inf();
     bits.set_sign(1);
     return bits;
   }
 
-  static T build_nan(UIntType v) {
+  static constexpr T build_nan(UIntType v) {
     FPBits<T> bits = inf();
     bits.set_mantissa(v);
     return T(bits);
   }
 };
 
 } // namespace fputil
 } // namespace __llvm_libc
 
 #ifdef SPECIAL_X86_LONG_DOUBLE
 #include "x86_64/LongDoubleBits.h"
 #endif
 
 #endif // LLVM_LIBC_SRC_SUPPORT_FPUTIL_FP_BITS_H

libc/src/__support/FPUtil/FloatProperties.h

 
 template <typename T> struct FloatProperties {};
 
 template <> struct FloatProperties<float> {
   typedef uint32_t BitsType;
   static_assert(sizeof(BitsType) == sizeof(float),
                 "Unexpected size of 'float' type.");
 
-  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) << 3;
+  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) * 8;
 
   static constexpr uint32_t MANTISSA_WIDTH = 23;
   static constexpr uint32_t EXPONENT_WIDTH = 8;
   static constexpr BitsType MANTISSA_MASK = (BitsType(1) << MANTISSA_WIDTH) - 1;
   static constexpr BitsType SIGN_MASK = BitsType(1)
                                         << (EXPONENT_WIDTH + MANTISSA_WIDTH);
   static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
   static constexpr uint32_t EXPONENT_BIAS = 127;
 
+  static constexpr BitsType EXP_MANT_MASK = MANTISSA_MASK + EXPONENT_MASK;
+  static_assert(EXP_MANT_MASK == ~SIGN_MASK,
+                "Exponent and mantissa masks are not as expected.");
+
   // If a number x is a NAN, then it is a quiet NAN if:
   //   QuietNaNMask & bits(x) != 0
   // Else, it is a signalling NAN.
   static constexpr BitsType QUIET_NAN_MASK = 0x00400000U;
 };
 
 template <> struct FloatProperties<double> {
   typedef uint64_t BitsType;
   static_assert(sizeof(BitsType) == sizeof(double),
                 "Unexpected size of 'double' type.");
 
-  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) << 3;
+  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) * 8;
 
   static constexpr uint32_t MANTISSA_WIDTH = 52;
   static constexpr uint32_t EXPONENT_WIDTH = 11;
   static constexpr BitsType MANTISSA_MASK = (BitsType(1) << MANTISSA_WIDTH) - 1;
   static constexpr BitsType SIGN_MASK = BitsType(1)
                                         << (EXPONENT_WIDTH + MANTISSA_WIDTH);
   static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
   static constexpr uint32_t EXPONENT_BIAS = 1023;
 
+  static constexpr BitsType EXP_MANT_MASK = MANTISSA_MASK + EXPONENT_MASK;
+  static_assert(EXP_MANT_MASK == ~SIGN_MASK,
+                "Exponent and mantissa masks are not as expected.");
+
   // If a number x is a NAN, then it is a quiet NAN if:
   //   QuietNaNMask & bits(x) != 0
   // Else, it is a signalling NAN.
   static constexpr BitsType QUIET_NAN_MASK = 0x0008000000000000ULL;
 };
 
 #if defined(LONG_DOUBLE_IS_DOUBLE)
 // Properties for numbers represented in 64 bits long double on Windows

 #elif defined(SPECIAL_X86_LONG_DOUBLE)
 // Properties for numbers represented in 80 bits long double on non-Windows x86
 // platforms.
 template <> struct FloatProperties<long double> {
   typedef __uint128_t BitsType;
   static_assert(sizeof(BitsType) == sizeof(long double),
                 "Unexpected size of 'long double' type.");
 
-  static constexpr uint32_t BIT_WIDTH = (sizeof(BitsType) << 3) - 48;
+  static constexpr uint32_t BIT_WIDTH = (sizeof(BitsType) * 8) - 48;
 
   static constexpr uint32_t MANTISSA_WIDTH = 63;
   static constexpr uint32_t EXPONENT_WIDTH = 15;
   static constexpr BitsType MANTISSA_MASK = (BitsType(1) << MANTISSA_WIDTH) - 1;
   static constexpr BitsType SIGN_MASK =
       BitsType(1) << (EXPONENT_WIDTH + MANTISSA_WIDTH + 1);
   static constexpr BitsType EXPONENT_MASK =
       ((BitsType(1) << EXPONENT_WIDTH) - 1) << (MANTISSA_WIDTH + 1);