ebevhan added inline comments.
================
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+ Val = Val.extend(DstWidth);
+ if (Upscaling)
----------------
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)
================
Comment at: lib/Basic/FixedPoint.cpp:58
+ if (!DstSema.isSigned && Val.isNegative()) {
+ Val = 0;
+ } else if (!(Masked == Mask || Masked == 0)) {
----------------
leonardchan wrote:
> 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?
Yes, even though it's undefined behavior to wrap, I think it's a good idea to
do as little work as possible for each operation and keep every operation as
simple as possible. This is to make it easier to codegen later, since it will
mean fewer operations, it will be simpler to understand the connection between
the semantics of the operations and the code emission and there will be better
'alignment' between generated code and constant evaluation results.
Obviously, if you invoke undefined behavior in runtime anything can happen, but
I think that the operations should behave "sensibly" in isolation.
================
Comment at: lib/Basic/FixedPoint.cpp:50
+ else
+ NewVal = 0;
+ } else {
----------------
If you move the `!DstSema.isSigned() && NewVal.isNegative()` block after the
mask check, you can avoid doing `= 0` twice.
================
Comment at: lib/Basic/FixedPoint.cpp:58
+ NewVal = NewVal.extOrTrunc(DstWidth);
+ return APFixedPoint(NewVal, DstSema);
+}
----------------
The signedness of NewVal is not being set here.
================
Comment at: unittests/Basic/FixedPointTest.cpp:380
+}
+
+// Check the value in a given fixed point sema overflows to the saturated max
----------------
Technically, neither CheckSaturatedConversion function checks whether or not
the result was correct, only that it saturated.
================
Comment at: unittests/Basic/FixedPointTest.cpp:506
+TEST(FixedPoint, AccConversionOverflow) {
+ // To SAcc max limit (65536)
+ CheckSaturatedConversionMax(getLAccSema(), Saturated(getAccSema()),
----------------
Acc?
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
