[libc-commits] [PATCH] D136799: [libc] Implement a high-precision floating point class.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

There is a similar class by name XFloat but it does not have addition operation: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/XFloat.h
Remove it if you don't like it.



================
Comment at: libc/src/__support/FPUtil/big_float.h:30
+// the mantissa.  So the real value that is stored is:
+//   real value = (-1)^sign * 2^exponent * (mantissa as unsigned integer)
+template <size_t MANTISSA_LENGTH> struct BigFloat {
----------------
Explain why this is better or preferred over the conventional exponent.


================
Comment at: libc/src/__support/FPUtil/big_float.h:42
+
+  // Assumption: a is normalized and not Inf/NaN.
+  BigFloat(double a) {
----------------
You can drop this assumption by using `NormalFloat`.


================
Comment at: libc/src/__support/FPUtil/big_float.h:180
+template <size_t LENGTH>
+constexpr BigFloat<LENGTH> quick_mul(const BigFloat<LENGTH> &a,
+                                     const BigFloat<LENGTH> &b) {
----------------
You should explain why these functiona have `quick_` prefix in their name, and why they are not member operators.


================
Comment at: libc/src/__support/UInt.h:25-28
+template <typename T> struct pair {
+  T lo = 0;
+  T hi = 0;
+};
----------------
michaelrj wrote:
> The `pair` class should probably be in a separate file.
Agree - you can choose to add a `cpp:pair` class.


================
Comment at: libc/src/__support/UInt.h:58
+
+constexpr inline pair<uint64_t> full_mul(uint64_t a, uint64_t b) {
+  pair<uint64_t> pa = split(a);
----------------
These functions just seem to me like specializations of the UInt `ful_add` and `ful_mul`. If you really need these separately, you need to add some commentary to justify.


================
Comment at: libc/src/__support/UInt.h:592
+
+  for (size_t i = 0; i < Bits1 / 64; ++i) {
+    util::pair<uint64_t> carry;
----------------
There could be a symbolic constant for this number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136799