[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 a comment.

Sorry for the delay in the review. I have gone through the history and it mostly looks good. I have a few comments about the cosmetics.



================
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;
----------------
Can this struct be removed now?


================
Comment at: libc/utils/FPUtil/FPBits.h:50
+  using FloatProp = FloatProperties<T>;
+  using UIntType = typename FloatProp::BitsType;
+
----------------
For consistency, I would name `UIntType` as `BitsType` here as well. But, you can choose to add a `TODO` here and do that "cleanup" in a follow up change.


================
Comment at: libc/utils/FPUtil/FPBits.h:58
+    UIntType getValue() const { return bits; }
+    void setValue(UIntType bits) { this->bits = bits; }
+
----------------
Why are these methods required? Probably because you got rid of the `integer` field of `FPBits`? If you add the rest of the methods to `FPBits` directly (see below), this should not be required as the `integer` / `bits` value can be directly set or read.


================
Comment at: libc/utils/FPUtil/FPBits.h:74
+
+    uint16_t getExponent() const {
+      return uint16_t((bits & FloatProp::exponentMask) >>
----------------
You should probably name this method `unbiasedExponent` to distinguish it from the real exponent value returned by `getExponent()`.


================
Comment at: libc/utils/FPUtil/FPBits.h:89
 
-  struct __attribute__((packed)) {
-    UIntType mantissa : MantissaWidth<T>::value;
-    uint16_t exponent : ExponentWidth<T>::value;
-    uint8_t sign : 1;
   } encoding;
   T val;
----------------
I would actually prefer if we got rid of `encoding` completely [1]. Adding the new methods to `FPBits` directly would reduce the verbosity when calling them. The integer value is currently called `integer`, but as @aeubanks suggests elsewhere, `bits` is more appropriate may be. However, to reduce the churn, you can choose to keep it as `integer` or `bits` depending on whats convenient to you.

[1] - We had a separate `encoding` field because it was essentially that when we could use bit-fields. Moreover, it helped us distinguish it with the other fields. With that gone, we can choose to remove it and eliminate that "complexity" and verbosity.


================
Comment at: libc/utils/FPUtil/FPBits.h:64
+      mantVal &= (FloatProp::mantissaMask);
+      assert((mantVal & ~(FloatProp::mantissaMask)) == 0);
+      bits &= ~(FloatProp::mantissaMask);
----------------
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), ...);
```


================
Comment at: libc/utils/FPUtil/FPBits.h:91
+
+    uint8_t getSign() const {
+      return uint8_t((bits & FloatProp::signMask) >> (FloatProp::bitWidth - 1));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105561