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

[Bevin Hansson via Phabricator via cfe-commits]

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