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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
