[libc-commits] [PATCH] D105153: [libc] Add on float properties for precision floating point numbers in FloatProperties.h

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Almost there. Thanks for your patience.



================
Comment at: libc/utils/FPUtil/FloatProperties.h:81
+      FloatProperties<double>::exponentMask;
+  static constexpr uint32_t exponentOffset =
+      FloatProperties<double>::exponentOffset;
----------------
The correct terminology is `exponentBias`. Since you are touching this, can you replace `exponentOffset` with `exponentBias` at all places in this file?


================
Comment at: libc/utils/FPUtil/FloatProperties.h:98
+  static constexpr BitsType signMask = BitsType(1)
+                                       << (exponentWidth + mantissaWidth);
+  static constexpr BitsType exponentMask =
----------------
See below about the implicit bit. So, this should include another ` + 1`;


================
Comment at: libc/utils/FPUtil/FloatProperties.h:101
+      signMask - BitsType(1) -
+      mantissaMask; // Takes care of bits used for padding also.
+  static constexpr uint32_t exponentOffset = 16383;
----------------
The 80-bit encoding has another quirk - the implicit "1" bit is explicit. It is at bit position 64 (ie. the 65th LS bit). See this for more information: https://en.wikipedia.org/wiki/Extended_precision.
So, this calculation here should also do the equivalent of another `- (BitsType(1) << 64)`. Or, a clear way could be to add a `implicitBitMask` and do just `- implicitBitMask`.

Or, another different way to do the same would be:

```
static constexpr BitsType exponentMask = ((BitsType(1) << exponentWidth) - 1) << (mantissaWidth + 1);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105153