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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/FPUtil/FPBits.h:64
+      mantVal &= (FloatProp::mantissaMask);
+      assert((mantVal & ~(FloatProp::mantissaMask)) == 0);
+      bits &= ~(FloatProp::mantissaMask);
----------------
aeubanks wrote:
> 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?
Ah, OK! I should have said `static_assert` in my comment. The general rule we follow is:

1. Include freestanding C headers if required.
2. Include the header file corresponding to the implementation if required. So, in fputils, we can include `math.h`.
3. Do not include other libc public headers unless they are related.

Some of the above are checked by the libc lint rules implemented as part of clang-tidy. They are only run on the full build builders: https://lab.llvm.org/buildbot/#/workers/120

So, we cannot include `assert.h` or use `assert`. Whether the value should be checked in the caller or the setter, there are a few algorithms which assume the setters are doing it (there is a deliberate intention to overflow the mantissa.) So, I would say do the masking in the setter to retain functionality. In practice, it isn't a big runtime penalty as the setting usually happens at the very end of a complex algorithm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105561