[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/Basic/FixedPoint.cpp:40
+  if (DstWidth > Val.getBitWidth())
+    Val = Val.extend(DstWidth);
+  if (Upscaling)
----------------
ebevhan wrote:
> It should be possible to replace this with `extOrTrunc` and move it below the saturation.
I don't think we can move the extension below saturation since saturation requires checking the bits above the dst width which may require extending and shifting beforehand.

Still leaving it above saturation may require checking for the width over using `extOrTrunc` to prevent truncating early before right shifting.


================
Comment at: lib/Basic/FixedPoint.cpp:58
+  if (!DstSema.isSigned && Val.isNegative()) {
+    Val = 0;
+  } else if (!(Masked == Mask || Masked == 0)) {
----------------
ebevhan wrote:
> If the value is unsigned and negative, we always zero it?
> 
> Also, this code always saturates the value regardless of whether the destination semantics call for it, no?
You're right. I did not test for this since I thought this would fall under undefined behavior converting a signed negative number to an unsigned number. I'll add a check for saturation since I imagine most people would expect modular wrap around, but do you think I should add a test case for this?


================
Comment at: lib/Basic/FixedPoint.cpp:73
+  if (DstWidth < Val.getBitWidth())
+    Val = Val.trunc(DstWidth);
+
----------------
ebevhan wrote:
> When we say 'width' do we mean the width - padding or the width including padding? What information does the semantics structure enshrine?
This width includes the padding. This is commented on over `struct fixedPointSema` where this width represents that of the underlying APInt, so if the sema is unsigned and hasUnsignedPadding is true, then in a provided APInt with this width, the `width-1` bit is padding.


Repository:
  rC Clang

https://reviews.llvm.org/D48661