[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9825
+    if (Result.isSigned() && !DstSigned) {
+      Overflow = Result < 0 || Result.ugt(DstMax);
+    } else if (Result.isUnsigned() && DstSigned) {
----------------
leonardchan wrote:
> ebevhan wrote:
> > The `Result < 0` is more clearly expressed as `Result.isNegative()` I think.
> Ah, so I ran into something similar with the patch preceding this in `APFixedPoint::convert()`. `isNegative()` is a method of `APInt` which doesn't care about signage. It just checks if the last bit is set. `Result < 0` calls `APSInt::operator<()` which cares about the sign and checks if this signed int is less than zero. 
> 
>  have no big problem with this, but if `Result.isNegative()` is more readable, I could also add `Result.isSigned()` which together effectively does the same thing as `Result < 0`.
This makes sense, but you're already checking if the value is signed in the line above, so it shouldn't be an issue.


================
Comment at: clang/test/Frontend/fixed_point_conversions.c:426
+  _Sat short _Accum sat_sa;
+  _Sat unsigned short _Accum sat_usa;
+
----------------
There are no tests here for what you get if you convert an integer to a fixed-point type with a larger integral part than the integer has.


================
Comment at: clang/test/Frontend/fixed_point_conversions.c:437
+  // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16
+  // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2
+
----------------
Conversions like this are a bit odd. There shouldn't be a need to upsize/upscale the container before the saturation, should there?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56900/new/

https://reviews.llvm.org/D56900