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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:100
+  static constexpr uint32_t exponentWidth = 15;
+  static constexpr BitsType mantissaMask = 0xffffffffffffffffU;
+  static constexpr BitsType signMask = (BitsType)0x00008000U << 64;
----------------
aeubanks wrote:
> No need to change here, but it'd be nice if we didn't have to have constants like `mantissaMask`, which is derivable from `mantissaWidth`, in each FloatProperties. Rather, we could have the user of `mantissaMask` calculate the mask itself, e.g. `((BitsType)1 << T::mantissaWidth) - 1`
> 
> Or something like
> ```
> template <typename T>
> struct FloatPropertiesExt {
>   static constexpr T::BitsType mantissaMask = ((BitsType)1 << T::mantissaWidth) - 1;
> };
> ```
> 
> and use `FloatPropertiesExt<T>::mantissaMask` where you'd previously use `FloatProperties<T>::mantissaMask`.
should this constant be ULL instead of U?


================
Comment at: libc/utils/FPUtil/FloatProperties.h:102
+  static constexpr BitsType signMask = (BitsType)0x00008000U << 64;
+  static constexpr BitsType exponentMask = ~(signMask | mantissaMask);
+  static constexpr uint32_t exponentOffset = 16383;
----------------
Will this mask also get garbage padding bits above 80 bits?  If it is the case then you might need to get the exact constant, something like signMask - BitType(1) - mantissaMask


================
Comment at: libc/utils/FPUtil/FloatProperties.h:123
+  static constexpr BitsType mantissaMask =
+      ((BitsType)0x0000ffffffffffffU << 64) | (BitsType)0xffffffffffffffffU;
+  static constexpr BitsType signMask = (BitsType)0x8000000000000000U << 64;
----------------
sivachandra wrote:
> Here and elsewhere in this patch, a more C++ way to do the compile time cast/conversion is to do it this way:
> 
> ```
> BitsType(...) << 64 | BitsType(...)
> ```
> 
> As opposed to the C style casts you have. The above style is probably also more readable.
Should these constants be ULL instead of U?  Same for the line below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105153