[PATCH] D134725: [ADT] Add support for more formats in APFixedPoint

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

> Prior to this patch APFixedPoint only support semantics where the LsbWeight is is negative and the Width is at least as large as -LsbWeight.
> This patch remove both those requirements.
> for example:
> with LsbWeight = 2, 12 would be represented as 3.
> with LsbWeight = -2, 12 would be represented as 48.

IIUC should the weights have opposite values in this example? Based off the code, it looks like scale is just negative. So with LsbWeight = -2, then the scale is 2, and 0x1100 (12) is actually 0x11.00 (3). Similarly, LsbWeight = 2, then the scale is -2, and 0x1100 (12) is actually 0x1100|00 (48). Weight doesn't seem to be mentioned in N1169 so I just want to make sure I understand correctly.



================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:41-44
   FixedPointSemantics(unsigned Width, unsigned Scale, bool IsSigned,
                       bool IsSaturated, bool HasUnsignedPadding)
-      : Width(Width), Scale(Scale), IsSigned(IsSigned),
+      : FixedPointSemantics(Width, Lsb{-static_cast<int>(Scale)}, IsSigned,
+                            IsSaturated, HasUnsignedPadding) {}
----------------
I *think* it should be ok to just have one constructor that takes an int for the LsbWeight.


================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:49
         IsSaturated(IsSaturated), HasUnsignedPadding(HasUnsignedPadding) {
-    assert(Width >= Scale && "Not enough room for the scale");
+    assert(isUInt<16>(Width) && isInt<13>(Weight.LsbWeight));
     assert(!(IsSigned && HasUnsignedPadding) &&
----------------
Perhaps place these constants in `constexpr`s before this class, have the bitfields use these `constexpr`s rather than literals, and have a `static_assert(sizeof(FixedPointSemantics) == 32)` after the class.


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

https://reviews.llvm.org/D134725