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

[Tyker via Phabricator via llvm-commits]

Tyker added a comment.

In D134725#3819217 <https://reviews.llvm.org/D134725#3819217>, @leonardchan wrote:

>> 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).

Yes, I used LsbWeight instead of Scale because negative scales looked weird to me and the representation of the semantics in other places use LsbWeight and MsbWeight.

> Weight doesn't seem to be mentioned in N1169 so I just want to make sure I understand correctly.

This is not related to N1169. as far as I know fixed points in N1169 don't have negative scales or width smaller then scale.



================
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) {}
----------------
leonardchan wrote:
> I *think* it should be ok to just have one constructor that takes an int for the LsbWeight.
C++ would not complain. but it would break the old API, and be hard to use correctly. whereas this change doesn't break the old API. but the syntax to build a FixedPointSemantics with an LsbWeight is heavier.


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

https://reviews.llvm.org/D134725