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

Arthur Eubanks via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue Jun 29 15:02:05 PDT 2021


aeubanks added a comment.

Typically all changes that add functionality should have tests. For example, a simple one where we construct some representation and mess around with the bits to make sure they work should be good enough.

@sivachandra are there existing tests for this sort of stuff?

Also, could you explain why you're adding the number of bits for exponents in the description? Something along the lines of not being able to use the sizeof the BitsType due to 80 long double vs 128 bit int.



================
Comment at: libc/utils/FPUtil/FloatProperties.h:63
+template <> struct FloatProperties<long double> {
+  typedef uint64_t BitsType;
+  static_assert(sizeof(BitsType) == sizeof(double),
----------------
no need to change it here since it matches the existing code, but it'd be nice to use `using BitsType = uint64_t` over a `typedef`, `using` is more modern and easier to read


================
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;
----------------
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`.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:153-154
+template <> struct FloatType<__uint128_t> {
+  static_assert(sizeof(__uint128_t) == sizeof(long double),
+                "Unexpected size of 'long double' type.");
+  typedef long double Type;
----------------
does this actually work? I thought long double was 80 bits on Linux/x86

what is the point of `FloatType` anyway? it's going to be ambiguous in the cases where we have `long double == double` since uint64_t will map to both


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105153



More information about the libc-commits mailing list