[libc-commits] [PATCH] D105561: [libc] Capture floating point encoding and arrange it sequentially in memory with structs

[Arthur Eubanks via Phabricator via libc-commits]

aeubanks added inline comments.


================
Comment at: libc/utils/FPUtil/FPBits.h:30
 
-#ifdef LONG_DOUBLE_IS_DOUBLE
-template <> struct MantissaWidth<long double> {
-  static constexpr unsigned value = MantissaWidth<double>::value;
+template <typename T> struct FPUIntType {
+  using Type = typename FloatProperties<T>::BitsType;
----------------
sivachandra wrote:
> Can this struct be removed now?
I'd say clean this, along with Exponent/MantissaWidth up in a later patch, there are lots of uses of these


================
Comment at: libc/utils/FPUtil/FPBits.h:64
+      mantVal &= (FloatProp::mantissaMask);
+      assert((mantVal & ~(FloatProp::mantissaMask)) == 0);
+      bits &= ~(FloatProp::mantissaMask);
----------------
sivachandra wrote:
> aeubanks wrote:
> > @sivachandra is this the right way to assert?
> Not sure what line this pertains to. But, after this change, the only assert that would be required is something like:
> 
> ```
> assert(sizeof(T) == sizeof(UIntVal), ...);
> ```
oh, I think the code was deleted for some reason

I spent a while debugging a failure Hedin was running into. Turns out sometimes when setting the mantissa/exponent, it was actually larger than the max value (IIRC at the very bottom of NormalFloat.h). With the struct bitfield it automatically took care of that, but here we needed to mask out the extra bits. The question is whether to do that in the setter or do that in the caller. If we do that in the setter we preserve the existing behavior. But I'm slightly in favor of forcing callers to handle that and adding an assert in the setter that the bits don't overflow. This is probably a bit faster since we don't have to do the bitmask on every setter, and may catch future issues.

The question about the assert was, is `assert(cond);` the right way to assert? As in in debug modes it'll run the assert and crash, and in release modes the assert won't be there for perf reasons. Is `<assert.h>` the right thing to include?


================
Comment at: libc/utils/FPUtil/FPBits.h:91
+
+    uint8_t getSign() const {
+      return uint8_t((bits & FloatProp::signMask) >> (FloatProp::bitWidth - 1));
----------------
sivachandra wrote:
> aeubanks wrote:
> > not sure if this should be a bool or a uint8_t...
> > let's see what the others say
> To which line was this comment originally for?
this was for `getSign()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105561