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

[Tue Ly via Phabricator via libc-commits]

lntue marked 6 inline comments as done.
lntue added inline comments.


================
Comment at: libc/src/__support/FPUtil/big_float.h:22
+namespace __llvm_libc {
+namespace fputil {
+
----------------
tschuett wrote:
> What is libc`s stand on nested namespaces?
I think we are totally fine with nested namespaces.  This is mostly my part of copy-pasting from other headers in the same directory.


================
Comment at: libc/src/__support/FPUtil/big_float.h:32
+template <size_t MANTISSA_LENGTH> struct BigFloat {
+  static_assert(MANTISSA_LENGTH >= 64 && MANTISSA_LENGTH % 8 == 0,
+                "Unsupported mantissa length.");
----------------
orex wrote:
> static_assert in UInt is stricter than this one (https://github.com/llvm/llvm-project/blob/7f93ae808634e33e4dc9bce753c909aa5f9a6eb4/libc/src/__support/UInt.h#L25) Probably you should remove or adjust this?
I was debating about letting this class using other uint types for the mantissa.  But it's probably better to just let the UInt class taking care of it.  Removed this `static_assert`.


================
Comment at: libc/src/__support/FPUtil/big_float.h:42
+
+  // Assumption: a is normalized and not Inf/NaN.
+  BigFloat(double a) {
----------------
sivachandra wrote:
> You can drop this assumption by using `NormalFloat`.
I was thinking about using `NormalFloat` but there are some difference in the expectation between them that will need some extra work to be done in order to merge this and `NormalFloat`:
- currently `NormalFloat` expected to use floating point type as the template parameter, while we want to explicitly control the bits


================
Comment at: libc/src/__support/FPUtil/big_float.h:84
+    // Move leading 1 (if non-zero) to the most significant bit of mantissa.
+    if (shift) {
+      e -= shift;
----------------
orex wrote:
> The if is not needed. The operations in the if statement probably can be even faster than check and conditional jump? 
The current `<<=` op in `UInt` is non-trivial and more expensive than a conditional jump.  Other option is to put the check for trivial case into `UInt` itself.


================
Comment at: libc/src/__support/FPUtil/big_float.h:123
+  using mtype = typename BigFloat<SHIFT_LENGTH>::mantissa_type;
+  return {{a.sign, a.exponent, static_cast<mtype>(a.mantissa)},
+          {a.sign, a.exponent + static_cast<int>(SHIFT_LENGTH),
----------------
orex wrote:
> Is this static cast by standard drop higher part of the variable?
Yes, it is specified for `unsigned integers`.


================
Comment at: libc/src/__support/FPUtil/big_float.h:129
+template <size_t LENGTH>
+constexpr BigFloat<LENGTH> quick_add(BigFloat<LENGTH> a, BigFloat<LENGTH> b) {
+  if (unlikely(a.is_zero()))
----------------
orex wrote:
> Am I understand right, that rounding and sign of zero in this function is not important?
Yes, for now its main purpose is to aid the computation of polynomial approximations.  So we don't pay too much attention on signed zeroes and rounding, which in turns giving us a tiny bit of performance gain.


================
Comment at: libc/src/__support/UInt.h:25-28
+template <typename T> struct pair {
+  T lo = 0;
+  T hi = 0;
+};
----------------
sivachandra wrote:
> michaelrj wrote:
> > The `pair` class should probably be in a separate file.
> Agree - you can choose to add a `cpp:pair` class.
I was debating about it, because the standard `std::pair` has members named `first` and `second`, while I want to named them `hi` and `lo` to signify the high part and the low part of the numbers.  I'm not sure what's the best way to have such distinction yet.  Maybe rename this to `cpp::number_pair` or something similar?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136799