[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:
> leonardchan wrote:
> > 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.
> I think the cases where saturation takes effect are limited to simply truncation, since when we upscale we automatically extend first to avoid shifting out bits. This means that the only situation where we need to check for saturation is right before we truncate, and we don't have to worry about anything happening when we upscale.
> 
> Do you have an example of when the extension is needed? (In case it wasn't clear, I mean the `NewVal = NewVal.extend(DstWidth);` extension, not the extension that is part of the upscale)
Ooooooh I see. Sorry this confused me for a bit. Thanks.


================
Comment at: unittests/Basic/FixedPointTest.cpp:380
+}
+
+// Check the value in a given fixed point sema overflows to the saturated max
----------------
ebevhan wrote:
> Technically, neither CheckSaturatedConversion function checks whether or not the result was correct, only that it saturated.
Could you elaborate on this? It should be correct that the value saturates to the respective min/max for the destination type right?


================
Comment at: unittests/Basic/FixedPointTest.cpp:506
+TEST(FixedPoint, AccConversionOverflow) {
+  // To SAcc max limit (65536)
+  CheckSaturatedConversionMax(getLAccSema(), Saturated(getAccSema()),
----------------
ebevhan wrote:
> Acc?
Short for Accum. Changed to `Accum` since it wasn't clear at first.


Repository:
  rC Clang

https://reviews.llvm.org/D48661