[libc][NFC] Simplify FloatProperties implementation #74821

gchatelet · 2023-12-08T10:28:53Z

This is a follow up on #74481 that migrates code from all specializations into FPCommonProperties (except for EXPLICIT_BIT_MASK in the X86_Binary80 specialization)

llvmbot · 2023-12-08T10:29:20Z

@llvm/pr-subscribers-libc

Author: Guillaume Chatelet (gchatelet)

Changes

This is a follow up on #74481 that migrates code from all specializations into FPCommonProperties (except for EXPLICIT_BIT_MASK in the X86_Binary80 specialization)

Full diff: https://github.com/llvm/llvm-project/pull/74821.diff

1 Files Affected:

(modified) libc/src/__support/FPUtil/FloatProperties.h (+140-90)

diff --git a/libc/src/__support/FPUtil/FloatProperties.h b/libc/src/__support/FPUtil/FloatProperties.h
index 59e261e1047f1..1bee06dd42575 100644
--- a/libc/src/__support/FPUtil/FloatProperties.h
+++ b/libc/src/__support/FPUtil/FloatProperties.h
@@ -10,7 +10,7 @@
 #define LLVM_LIBC_SRC___SUPPORT_FPUTIL_FLOATPROPERTIES_H
 
 #include "src/__support/UInt128.h"
-#include "src/__support/macros/attributes.h"       // LIBC_INLINE
+#include "src/__support/macros/attributes.h" // LIBC_INLINE, LIBC_INLINE_VAR
 #include "src/__support/macros/properties/float.h" // LIBC_COMPILER_HAS_FLOAT128
 
 #include <stdint.h>
@@ -27,116 +27,166 @@ enum class FPType {
   X86_Binary80,
 };
 
-template <FPType> struct FPProperties {};
-template <> struct FPProperties<FPType::IEEE754_Binary32> {
-  typedef uint32_t BitsType;
+// For now 'FPEncoding', 'FPBaseProperties' and 'FPCommonProperties' are
+// implementation details.
+namespace internal {
 
-  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) * 8;
+// The type of encoding for supported floating point types.
+enum class FPEncoding {
+  IEEE754,
+  X86_ExtendedPrecision,
+};
 
-  static constexpr uint32_t MANTISSA_WIDTH = 23;
-  // The mantissa precision includes the implicit bit.
-  static constexpr uint32_t MANTISSA_PRECISION = MANTISSA_WIDTH + 1;
-  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;
+template <FPType> struct FPBaseProperties {};
 
-  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.");
+template <> struct FPBaseProperties<FPType::IEEE754_Binary16> {
+  using UIntType = uint16_t;
+  LIBC_INLINE_VAR static constexpr int TOTAL_BITS = 16;
+  LIBC_INLINE_VAR static constexpr int SIG_BITS = 10;
+  LIBC_INLINE_VAR static constexpr int EXP_BITS = 5;
+  LIBC_INLINE_VAR static constexpr auto ENCODING = FPEncoding::IEEE754;
+};
 
-  // 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 FPBaseProperties<FPType::IEEE754_Binary32> {
+  using UIntType = uint32_t;
+  LIBC_INLINE_VAR static constexpr int TOTAL_BITS = 32;
+  LIBC_INLINE_VAR static constexpr int SIG_BITS = 23;
+  LIBC_INLINE_VAR static constexpr int EXP_BITS = 8;
+  LIBC_INLINE_VAR static constexpr auto ENCODING = FPEncoding::IEEE754;
 };
 
-template <> struct FPProperties<FPType::IEEE754_Binary64> {
-  typedef uint64_t BitsType;
+template <> struct FPBaseProperties<FPType::IEEE754_Binary64> {
+  using UIntType = uint64_t;
+  LIBC_INLINE_VAR static constexpr int TOTAL_BITS = 64;
+  LIBC_INLINE_VAR static constexpr int SIG_BITS = 52;
+  LIBC_INLINE_VAR static constexpr int EXP_BITS = 11;
+  LIBC_INLINE_VAR static constexpr auto ENCODING = FPEncoding::IEEE754;
+};
 
-  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) * 8;
+template <> struct FPBaseProperties<FPType::IEEE754_Binary128> {
+  using UIntType = UInt128;
+  LIBC_INLINE_VAR static constexpr int TOTAL_BITS = 128;
+  LIBC_INLINE_VAR static constexpr int SIG_BITS = 112;
+  LIBC_INLINE_VAR static constexpr int EXP_BITS = 15;
+  LIBC_INLINE_VAR static constexpr auto ENCODING = FPEncoding::IEEE754;
+};
 
-  static constexpr uint32_t MANTISSA_WIDTH = 52;
-  static constexpr uint32_t MANTISSA_PRECISION = MANTISSA_WIDTH + 1;
-  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;
+template <> struct FPBaseProperties<FPType::X86_Binary80> {
+  using UIntType = UInt128;
+  LIBC_INLINE_VAR static constexpr int TOTAL_BITS = 80;
+  LIBC_INLINE_VAR static constexpr int SIG_BITS = 64;
+  LIBC_INLINE_VAR static constexpr int EXP_BITS = 15;
+  LIBC_INLINE_VAR static constexpr auto ENCODING =
+      FPEncoding::X86_ExtendedPrecision;
+};
 
-  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.");
+// TODO: Move this utility elsewhere.
+template <typename T, size_t count> static constexpr T mask_trailing_ones() {
+  static_assert(cpp::is_unsigned_v<T>);
+  constexpr unsigned t_bits = CHAR_BIT * sizeof(T);
+  static_assert(count <= t_bits && "Invalid bit index");
+  return count == 0 ? 0 : (T(-1) >> (t_bits - count));
+}
+
+// Derives more properties from 'FPBaseProperties' above.
+// This class serves as a halfway point between 'FPBaseProperties' and
+// 'FPProperties' below.
+template <FPType fp_type>
+struct FPCommonProperties : private FPBaseProperties<fp_type> {
+private:
+  using UP = FPBaseProperties<fp_type>;
+  using UP::EXP_BITS;
+  using UP::SIG_BITS;
+  using UP::TOTAL_BITS;
+  using UIntType = typename UP::UIntType;
+
+  LIBC_INLINE_VAR static constexpr bool HAS_EXPLICIT_SIG_BIT =
+      UP::ENCODING == FPEncoding::X86_ExtendedPrecision;
+  LIBC_INLINE_VAR static constexpr int EXPLICIT_SIG_BITS =
+      HAS_EXPLICIT_SIG_BIT ? 1 : 0;
+
+  LIBC_INLINE_VAR static constexpr int STORAGE_BITS =
+      sizeof(UIntType) * CHAR_BIT;
+  static_assert(STORAGE_BITS >= TOTAL_BITS);
+
+  // The number of bits to represent sign.
+  // For documentation purpose, always 1.
+  LIBC_INLINE_VAR static constexpr int SIGN_BITS = 1;
+  static_assert(SIGN_BITS + EXP_BITS + SIG_BITS == TOTAL_BITS);
+
+  // The exponent bias. Always positive.
+  LIBC_INLINE_VAR static constexpr int32_t EXP_BIAS =
+      (1U << (EXP_BITS - 1U)) - 1U;
+  static_assert(EXP_BIAS > 0);
+
+  // Shifts
+  LIBC_INLINE_VAR static constexpr int SIG_MASK_SHIFT = 0;
+  LIBC_INLINE_VAR static constexpr int EXP_MASK_SHIFT = SIG_BITS;
+  LIBC_INLINE_VAR static constexpr int SIGN_MASK_SHIFT = SIG_BITS + EXP_BITS;
+
+  // Masks
+  LIBC_INLINE_VAR static constexpr UIntType SIG_MASK =
+      mask_trailing_ones<UIntType, SIG_BITS>() << SIG_MASK_SHIFT;
+  LIBC_INLINE_VAR static constexpr UIntType EXP_MASK =
+      mask_trailing_ones<UIntType, EXP_BITS>() << EXP_MASK_SHIFT;
+  LIBC_INLINE_VAR static constexpr UIntType SIGN_MASK_ =
+      mask_trailing_ones<UIntType, SIGN_BITS>() << SIGN_MASK_SHIFT;
+  LIBC_INLINE_VAR static constexpr UIntType FP_MASK =
+      mask_trailing_ones<UIntType, TOTAL_BITS>();
+  static_assert((SIG_MASK & EXP_MASK & SIGN_MASK_) == 0, "masks disjoint");
+  static_assert((SIG_MASK | EXP_MASK | SIGN_MASK_) == FP_MASK, "masks covers");
+
+  //
+  LIBC_INLINE_VAR static constexpr int FRACTION_BITS =
+      SIG_BITS - EXPLICIT_SIG_BITS;
+  LIBC_INLINE_VAR static constexpr UIntType QNAN_MASK = UIntType(1)
+                                                        << (FRACTION_BITS - 1);
+  LIBC_INLINE_VAR static constexpr UIntType SNAN_MASK = UIntType(1)
+                                                        << (FRACTION_BITS - 2);
+
+public:
+  // Public facing API to keep the change local to this file.
+  using BitsType = UIntType;
+
+  LIBC_INLINE_VAR static constexpr uint32_t BIT_WIDTH = TOTAL_BITS;
+  LIBC_INLINE_VAR static constexpr uint32_t MANTISSA_WIDTH = FRACTION_BITS;
+  LIBC_INLINE_VAR static constexpr uint32_t MANTISSA_PRECISION =
+      MANTISSA_WIDTH + 1;
+  LIBC_INLINE_VAR static constexpr BitsType MANTISSA_MASK =
+      (BitsType(1) << MANTISSA_WIDTH) - 1;
+  LIBC_INLINE_VAR static constexpr uint32_t EXPONENT_WIDTH = EXP_BITS;
+  LIBC_INLINE_VAR static constexpr uint32_t EXPONENT_BIAS = EXP_BIAS;
+  LIBC_INLINE_VAR static constexpr BitsType SIGN_MASK = SIGN_MASK_;
+  LIBC_INLINE_VAR static constexpr BitsType EXPONENT_MASK = EXP_MASK;
+  LIBC_INLINE_VAR static constexpr BitsType EXP_MANT_MASK = EXP_MASK | SIG_MASK;
 
   // 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;
+  static constexpr BitsType QUIET_NAN_MASK = QNAN_MASK;
 };
 
-// Properties for numbers represented in 80 bits long double on non-Windows x86
-// platforms.
-template <> struct FPProperties<FPType::X86_Binary80> {
-  typedef UInt128 BitsType;
+} // namespace internal
 
-  static constexpr uint32_t BIT_WIDTH = (sizeof(BitsType) * 8) - 48;
-  static constexpr BitsType FULL_WIDTH_MASK = ((BitsType(1) << BIT_WIDTH) - 1);
+template <FPType fp_type>
+struct FPProperties : public internal::FPCommonProperties<fp_type> {};
 
-  static constexpr uint32_t MANTISSA_WIDTH = 63;
-  static constexpr uint32_t MANTISSA_PRECISION = MANTISSA_WIDTH + 1;
-  static constexpr uint32_t EXPONENT_WIDTH = 15;
-  static constexpr BitsType MANTISSA_MASK = (BitsType(1) << MANTISSA_WIDTH) - 1;
+// ----------------
+// Work In Progress
+// ----------------
+// The 'FPProperties' template specializations below are being slowly replaced
+// with properties from 'FPCommonProperties' above. Once specializations are
+// empty, 'FPProperties' declaration can be fully replace with
+// 'FPCommonProperties' implementation.
 
+// Properties for numbers represented in 80 bits long double on non-Windows x86
+// platforms.
+template <>
+struct FPProperties<FPType::X86_Binary80>
+    : public internal::FPCommonProperties<FPType::X86_Binary80> {
   // The x86 80 bit float represents the leading digit of the mantissa
   // explicitly. This is the mask for that bit.
   static constexpr BitsType EXPLICIT_BIT_MASK = (BitsType(1) << MANTISSA_WIDTH);
-
-  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);
-  static constexpr uint32_t EXPONENT_BIAS = 16383;
-
-  static constexpr BitsType EXP_MANT_MASK =
-      MANTISSA_MASK | EXPLICIT_BIT_MASK | EXPONENT_MASK;
-  static_assert(EXP_MANT_MASK == (~SIGN_MASK & FULL_WIDTH_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 = BitsType(1)
-                                             << (MANTISSA_WIDTH - 1);
-};
-
-// Properties for numbers represented in 128 bits long double on non x86
-// platform.
-template <> struct FPProperties<FPType::IEEE754_Binary128> {
-  typedef UInt128 BitsType;
-
-  static constexpr uint32_t BIT_WIDTH = sizeof(BitsType) << 3;
-
-  static constexpr uint32_t MANTISSA_WIDTH = 112;
-  static constexpr uint32_t MANTISSA_PRECISION = MANTISSA_WIDTH + 1;
-  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);
-  static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
-  static constexpr uint32_t EXPONENT_BIAS = 16383;
-
-  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 = BitsType(1)
-                                             << (MANTISSA_WIDTH - 1);
 };
 
 //-----------------------------------------------------------------------------

gchatelet · 2023-12-08T10:30:12Z

The interesting change is a416d0b.

lntue · 2023-12-08T14:34:20Z

libc/src/__support/FPUtil/FloatProperties.h

+  LIBC_INLINE_VAR static constexpr BitsType MANTISSA_MASK =
+      (BitsType(1) << MANTISSA_WIDTH) - 1;
+  LIBC_INLINE_VAR static constexpr uint32_t EXPONENT_WIDTH = EXP_BITS;
+  LIBC_INLINE_VAR static constexpr uint32_t EXPONENT_BIAS = EXP_BIAS;


type mismatched between EXPONENT_BIAS and EXP_BIAS.

I made it obvious with a static_cast.
EXP_BIAS is int32_t by design since most of the time we'll be manipulating the exponent as a signed integer. It becomes unsigned only when encoding the bits. For the purpose of this patch I kept EXPONENT_BIAS as uint32_t. The cast is a noop because we've checked above that it's positive static_assert(EXP_BIAS > 0);.

I did a quick run through the code base and indeed most use of EXPONENT_BIAS involves signed arithmetic so it seems favorable to change its type to int32_t in the long run - not in this patch though.

move to int32_t will be done in #75046.

lntue · 2023-12-08T14:46:13Z

libc/src/__support/FPUtil/FloatProperties.h

+  //
+  LIBC_INLINE_VAR static constexpr int FRACTION_BITS =
+      SIG_BITS - EXPLICIT_SIG_BITS;
+  LIBC_INLINE_VAR static constexpr UIntType QNAN_MASK = UIntType(1)


I think in order for QNAN and SNAN work for both IEEE 754 and X86_FLOAT80, the nan masks should be

QNAN_MASK = (1 << SIGN_MASK_SHIFT) - (1 << (MANTISSA_WIDTH - 1)) SNAN_MASK = (1 << SIGN_MASK_SHIFT) - (1 << MANTISSA_WIDTH)

Then the detections are

is_quiet_nan(x) = ((bits(x) & QNAN_MASK) == QNAN_MASK) is_signaling_nan(x) = ((bits(x) & QNAN_MASK) == SNAN_MASK)

I made a quick bit printer https://godbolt.org/z/qaK99ebKx and I think I've messed up indeed. That said tests are green... 😅 Let me fix it.

libc/src/__support/FPUtil/FloatProperties.h

lntue · 2023-12-08T17:55:31Z

libc/src/__support/FPUtil/FloatProperties.h

-                "Exponent and mantissa masks are not as expected.");
+  LIBC_INLINE_VAR static constexpr UIntType QNAN_MASK =
+      UP::ENCODING == FPEncoding::X86_ExtendedPrecision
+          ? bit_at(SIG_BITS - 1) | bit_at(SIG_BITS - 2) // 0b1100...


Is there a reason for not including the exponent field to (Q|S)NAN_MASK?

I don't quite know what to do with (Q|S)NAN_MASK in the future, the purpose of this patch is to be a NFC and to retain the original semantics of QUIET_NAN_MASK.

llvm-project/libc/src/__support/FPUtil/FloatProperties.h

Lines 138 to 139 in 75193b1

static constexpr BitsType QUIET_NAN_MASK = BitsType(1)

<< (MANTISSA_WIDTH - 1);

The current documentation says

llvm-project/libc/src/__support/FPUtil/FloatProperties.h

Lines 135 to 137 in 75193b1

// If a number x is a NAN, then it is a quiet NAN if:

// QuietNaNMask & bits(x) != 0

// Else, it is a signalling NAN.

So it seems that being a NaN is a prerequisite for using this mask. Now I don't mind adding the exponent mask as well if you feel like it. Let me know!

llvmbot added the libc label Dec 8, 2023

gchatelet requested a review from lntue December 8, 2023 10:29

lntue reviewed Dec 8, 2023

View reviewed changes

gchatelet requested a review from lntue December 8, 2023 16:08

gchatelet changed the title ~~[libc][NFC] Simplify FloatProperties implementation #2~~ [libc][NFC] Simplify FloatProperties implementation Dec 8, 2023

lntue reviewed Dec 8, 2023

View reviewed changes

libc/src/__support/FPUtil/FloatProperties.h Show resolved Hide resolved

lntue reviewed Dec 8, 2023

View reviewed changes

gchatelet requested a review from lntue December 11, 2023 09:24

lntue approved these changes Dec 11, 2023

View reviewed changes

[libc][NFC] Simplify FloatProperties implementation

32b53e8

gchatelet force-pushed the simplify_float_properties2 branch from 38e1d82 to 32b53e8 Compare December 11, 2023 16:34

gchatelet merged commit e4f3ec2 into llvm:main Dec 11, 2023

gchatelet deleted the simplify_float_properties2 branch December 11, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc][NFC] Simplify FloatProperties implementation #74821

[libc][NFC] Simplify FloatProperties implementation #74821

Uh oh!

gchatelet commented Dec 8, 2023

Uh oh!

llvmbot commented Dec 8, 2023

Uh oh!

gchatelet commented Dec 8, 2023

Uh oh!

lntue Dec 8, 2023

Uh oh!

gchatelet Dec 8, 2023

Uh oh!

gchatelet Dec 8, 2023 •

edited

Loading

Uh oh!

gchatelet Dec 11, 2023

Uh oh!

lntue Dec 8, 2023 •

edited

Loading

Uh oh!

gchatelet Dec 8, 2023 •

edited

Loading

Uh oh!

Uh oh!

lntue Dec 8, 2023

Uh oh!

gchatelet Dec 11, 2023

Uh oh!

Uh oh!

	static constexpr BitsType QUIET_NAN_MASK = BitsType(1)
	<< (MANTISSA_WIDTH - 1);

	// If a number x is a NAN, then it is a quiet NAN if:
	// QuietNaNMask & bits(x) != 0
	// Else, it is a signalling NAN.

[libc][NFC] Simplify FloatProperties implementation #74821

[libc][NFC] Simplify FloatProperties implementation #74821

Uh oh!

Conversation

gchatelet commented Dec 8, 2023

Uh oh!

llvmbot commented Dec 8, 2023

Uh oh!

gchatelet commented Dec 8, 2023

Uh oh!

lntue Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

gchatelet Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

gchatelet Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchatelet Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

lntue Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchatelet Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lntue Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

gchatelet Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gchatelet Dec 8, 2023 •

edited

Loading

lntue Dec 8, 2023 •

edited

Loading

gchatelet Dec 8, 2023 •

edited

Loading