[libc] add FXBits class

[michaelrj-google]

The FXBits class is what will be used to modify fixed point numbers on a
bit level. This patch adds a basic implementation as well as basic
tests.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Michael Jones (michaelrj-google)

Changes

The FXBits class is what will be used to modify fixed point numbers on a
bit level. This patch adds a basic implementation as well as basic
tests.

---

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

6 Files Affected:

• (modified) libc/src/__support/fixed_point/CMakeLists.txt (+2)
• (modified) libc/src/__support/fixed_point/fx_bits.h (+70)
• (modified) libc/test/src/__support/CMakeLists.txt (+1)
• (modified) libc/test/src/__support/FPUtil/fpbits_test.cpp (+1-1)
• (added) libc/test/src/__support/fixed_point/CMakeLists.txt (+13)
• (added) libc/test/src/__support/fixed_point/fx_bits_test.cpp (+82)

diff --git a/libc/src/__support/fixed_point/CMakeLists.txt b/libc/src/__support/fixed_point/CMakeLists.txt
index 4dac9b33325adb..4203e5914acb60 100644
--- a/libc/src/__support/fixed_point/CMakeLists.txt
+++ b/libc/src/__support/fixed_point/CMakeLists.txt
@@ -17,4 +17,6 @@ add_header_library(
     libc.include.llvm-libc-macros.stdfix_macros
     libc.src.__support.macros.attributes
     libc.src.__support.macros.optimization
+    libc.src.__support.CPP.type_traits
+    libc.src.__support.CPP.bit
 )
diff --git a/libc/src/__support/fixed_point/fx_bits.h b/libc/src/__support/fixed_point/fx_bits.h
index ad3a6fc97c0a1f..b295220044ddfb 100644
--- a/libc/src/__support/fixed_point/fx_bits.h
+++ b/libc/src/__support/fixed_point/fx_bits.h
@@ -10,6 +10,8 @@
 #define LLVM_LIBC_SRC___SUPPORT_FIXEDPOINT_FXBITS_H
 
 #include "include/llvm-libc-macros/stdfix-macros.h"
+#include "src/__support/CPP/bit.h"
+#include "src/__support/CPP/type_traits.h"
 #include "src/__support/macros/attributes.h"   // LIBC_INLINE
 #include "src/__support/macros/optimization.h" // LIBC_UNLIKELY
 
@@ -19,6 +21,74 @@
 
 namespace LIBC_NAMESPACE::fixed_point {
 
+template <typename T> struct FXBits {
+private:
+  using fx_rep = FXRep<T>;
+  using StorageType = typename fx_rep::StorageType;
+
+  StorageType value;
+
+  static_assert(fx_rep::FRACTION_LEN > 0);
+
+  static constexpr size_t FRACTION_OFFSET = 0; // Just for completeness
+  static constexpr size_t INTEGRAL_OFFSET = fx_rep::FRACTION_LEN;
+  static constexpr size_t SIGN_OFFSET =
+      fx_rep::SIGN_LEN == 0
+          ? 0
+          : ((sizeof(StorageType) * CHAR_BIT) - fx_rep::SIGN_LEN);
+
+  static constexpr StorageType FRACTION_MASK =
+      (StorageType(1) << fx_rep::FRACTION_LEN) - 1;
+  static constexpr StorageType INTEGRAL_MASK =
+      ((StorageType(1) << fx_rep::INTEGRAL_LEN) - 1) << INTEGRAL_OFFSET;
+  static constexpr StorageType SIGN_MASK =
+      (fx_rep::SIGN_LEN == 0 ? 0 : StorageType(1) << SIGN_OFFSET);
+
+public:
+  LIBC_INLINE constexpr FXBits() = default;
+
+  template <typename XType> LIBC_INLINE constexpr explicit FXBits(XType x) {
+    using Unqual = typename cpp::remove_cv_t<XType>;
+    if constexpr (cpp::is_same_v<Unqual, T>) {
+      value = cpp::bit_cast<StorageType>(x);
+    } else if constexpr (cpp::is_same_v<Unqual, StorageType>) {
+      value = x;
+    } else {
+      // We don't want accidental type promotions/conversions, so we require
+      // exact type match.
+      static_assert(cpp::always_false<XType>);
+    }
+  }
+
+  LIBC_INLINE constexpr StorageType get_fraction() {
+    return (value & FRACTION_MASK) >> FRACTION_OFFSET;
+  }
+
+  LIBC_INLINE constexpr StorageType get_integral() {
+    return (value & INTEGRAL_MASK) >> INTEGRAL_OFFSET;
+  }
+
+  LIBC_INLINE constexpr StorageType get_sign() {
+    return (value & SIGN_MASK) >> SIGN_OFFSET;
+  }
+
+  LIBC_INLINE constexpr void set_fraction(StorageType fraction) {
+    value = (value & (~FRACTION_MASK)) |
+            ((fraction << FRACTION_OFFSET) & FRACTION_MASK);
+  }
+
+  LIBC_INLINE constexpr void set_integral(StorageType integral) {
+    value = (value & (~INTEGRAL_MASK)) |
+            ((integral << INTEGRAL_OFFSET) & INTEGRAL_MASK);
+  }
+
+  LIBC_INLINE constexpr void set_sign(StorageType sign) {
+    value = (value & (~SIGN_MASK)) | ((sign << SIGN_OFFSET) & SIGN_MASK);
+  }
+
+  LIBC_INLINE constexpr T get_val() const { return cpp::bit_cast<T>(value); }
+};
+
 template <typename T> LIBC_INLINE constexpr T abs(T x) {
   using FXRep = FXRep<T>;
   if constexpr (FXRep::SIGN_LEN == 0)
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 850f5385ed3431..d360f0e1d677c2 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -186,4 +186,5 @@ add_subdirectory(File)
 add_subdirectory(RPC)
 add_subdirectory(OSUtil)
 add_subdirectory(FPUtil)
+add_subdirectory(fixed_point)
 add_subdirectory(HashTable)
diff --git a/libc/test/src/__support/FPUtil/fpbits_test.cpp b/libc/test/src/__support/FPUtil/fpbits_test.cpp
index 1c8a1c5b9d4cee..c5fba17904eb07 100644
--- a/libc/test/src/__support/FPUtil/fpbits_test.cpp
+++ b/libc/test/src/__support/FPUtil/fpbits_test.cpp
@@ -1,4 +1,4 @@
-//===-- Unittests for the DyadicFloat class -------------------------------===//
+//===-- Unittests for the FPBits class ------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
diff --git a/libc/test/src/__support/fixed_point/CMakeLists.txt b/libc/test/src/__support/fixed_point/CMakeLists.txt
new file mode 100644
index 00000000000000..a39d4af87527de
--- /dev/null
+++ b/libc/test/src/__support/fixed_point/CMakeLists.txt
@@ -0,0 +1,13 @@
+add_custom_target(libc-fixed-point-tests)
+
+add_libc_test(
+  fx_bits_test
+  SUITE
+    libc-fixed-point-tests
+  SRCS
+    fx_bits_test.cpp
+  DEPENDS
+    libc.src.__support.fixed_point.fx_bits
+    # libc.src.__support.fixed_point.fx_bits_str TODO: make this
+    libc.src.__support.integer_literals
+)
diff --git a/libc/test/src/__support/fixed_point/fx_bits_test.cpp b/libc/test/src/__support/fixed_point/fx_bits_test.cpp
new file mode 100644
index 00000000000000..a168556351c2ce
--- /dev/null
+++ b/libc/test/src/__support/fixed_point/fx_bits_test.cpp
@@ -0,0 +1,82 @@
+//===-- Unittests for the FXBits class ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "include/llvm-libc-macros/stdfix-macros.h"
+
+#include "src/__support/fixed_point/fx_bits.h"
+// #include "src/__support/FPUtil/fx_bits_str.h"
+#include "src/__support/integer_literals.h"
+#include "test/UnitTest/Test.h"
+
+using LIBC_NAMESPACE::fixed_point::FXBits;
+using LIBC_NAMESPACE::fixed_point::FXRep;
+
+using LIBC_NAMESPACE::operator""_u8;
+using LIBC_NAMESPACE::operator""_u16;
+using LIBC_NAMESPACE::operator""_u32;
+using LIBC_NAMESPACE::operator""_u64;
+using LIBC_NAMESPACE::operator""_u128;
+
+TEST(LlvmLibcFxBitsTest, FXBits_UnsignedShortFract) {
+  auto bits_var = FXBits<unsigned short fract>(0x00_u8);
+
+  EXPECT_EQ(bits_var.get_sign(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_integral(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_fraction(), 0x00_u8);
+
+  // Since an unsigned fract has no sign or integral components, setting either
+  // should have no effect.
+
+  bits_var.set_sign(1);
+
+  EXPECT_EQ(bits_var.get_sign(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_integral(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_fraction(), 0x00_u8);
+
+  bits_var.set_integral(0xab);
+
+  EXPECT_EQ(bits_var.get_sign(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_integral(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_fraction(), 0x00_u8);
+
+  // but setting the fraction should work
+
+  bits_var.set_fraction(0xcd);
+
+  EXPECT_EQ(bits_var.get_sign(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_integral(), 0x00_u8);
+  EXPECT_EQ(bits_var.get_fraction(), 0xcd_u8);
+}
+
+TEST(LlvmLibcFxBitsTest, FXBits_ShortAccum) {
+  auto bits_var = FXBits<short accum>(0b0'00000000'0000000_u16);
+
+  EXPECT_EQ(bits_var.get_sign(), 0x0000_u16);
+  EXPECT_EQ(bits_var.get_integral(), 0x0000_u16);
+  EXPECT_EQ(bits_var.get_fraction(), 0x0000_u16);
+
+  bits_var.set_sign(0xffff); // one sign bit used
+
+  EXPECT_EQ(bits_var.get_sign(), 0x0001_u16);
+  EXPECT_EQ(bits_var.get_integral(), 0x0000_u16);
+  EXPECT_EQ(bits_var.get_fraction(), 0x0000_u16);
+
+  bits_var.set_integral(0xabcd_u16); // 8 integral bits used
+
+  EXPECT_EQ(bits_var.get_sign(), 0x0001_u16);
+  EXPECT_EQ(bits_var.get_integral(), 0x00cd_u16);
+  EXPECT_EQ(bits_var.get_fraction(), 0x0000_u16);
+
+  bits_var.set_fraction(0x21fe_u16); // 7 fract bits used
+
+  EXPECT_EQ(bits_var.get_sign(), 0x0001_u16);
+  EXPECT_EQ(bits_var.get_integral(), 0x00cd_u16);
+  EXPECT_EQ(bits_var.get_fraction(), 0x007e_u16);
+}
+
+// TODO: more types

[lntue]

Also do we need to annotate LIBC_INLINE_VAR for these?

[michaelrj-google]

we shouldn't need LIBC_INLINE_VAR for any constexpr variables. It's a linkage problem and if the number is a compile time constant the linker shouldn't need to touch it as far as I know.

[lntue]

Maybe get_sign and set_sign should take boolean. Or you can refactor struct Sign from https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/FPBits.h#L37 to share with both FXBits and FPBits.

[gchatelet]

I think it's indeed the right time to extract Sign and make it a first class citizen in LLVM libc.

[michaelrj-google]

I'm going to just use boolean for now since it's simple, but I will add a todo to move to Sign in a future patch.

[gchatelet]

A few nits

[michaelrj-google]

comments have been addressed and I rebased

[michaelrj-google]

we shouldn't need LIBC_INLINE_VAR for any constexpr variables. It's a linkage problem and if the number is a compile time constant the linker shouldn't need to touch it as far as I know.

[michaelrj-google]

I'm going to just use boolean for now since it's simple, but I will add a todo to move to Sign in a future patch.