[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 inline comments.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:94
+
+  static constexpr uint32_t mantissaWidth = 64;
+  static constexpr uint32_t exponentWidth = 15;
----------------
Ah! I missed that you made this 64 in which case some of my comments below are incorrect. I think our algorithms expect this to be 63 as listed here: https://github.com/llvm/llvm-project/blob/main/libc/utils/FPUtil/LongDoubleBitsX86.h#L20.


================
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;
----------------
sivachandra wrote:
> 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);
> ```
If `mantissaWidth` is 64, I think what you have is correct. But, if `mantissaWidth` is 63, you will have to do the additional adjustment I pointed out above. A correction to my comment above: the implicit bit is the 64th least significant bit and not the 65th least significant bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105153