[PATCH] D57553: [Fixed Point Arithmetic] Avoid resizing for types with the same width

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D57553#1381920 <https://reviews.llvm.org/D57553#1381920>, @leonardchan wrote:

> In regards to solving the problem of resizing for int conversions, I'm starting to think that we will need that initial resize since if we want to retain the min-max pattern for all conversions, then it would be necessary to extend and shift when converting from an int to fixed point. Otherwise, I think we'd have the initial pattern I proposed where we check against the source value, but not have this pattern.


Yes, I'm starting to think so too. It's just not possible to not resize and keep the minmax pattern at the same time. We can't upshift without resizing first (because any bits above the scale can affect saturation), and if we wanted to saturate purely on the non-rescaled value, then the (mandatory) rescaling after saturation could destroy the possibly saturated result (since it would shift in zeroes from the right if upscaling, which breaks the result if it saturated to max).

Sorry for going down this path, that was rather pointless.



================
Comment at: clang/test/Frontend/fixed_point_conversions.c:194
+  // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 -2147483648, i32 [[RESULT]]
+  // DEFAULT-NEXT: store i32 [[RESULT2]], i32* %sat_lf, align 4
 
----------------
This seems off. We're upshifting, then saturating on the upshifted value. But the top bits could have been shifted out, so the result might not be correct.

We can't upshift before saturating if the upshift could shift out bits.

If the upshift is moved after the saturation, then the saturation no longer works since it would be upshifting zeros into the saturation result, which is also wrong.


================
Comment at: clang/test/Frontend/fixed_point_conversions.c:222
+  // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 0, i32 [[ACCUM]]
+  // DEFAULT-NEXT: store i32 [[RESULT2]], i32* %sat_ua, align 4
   // SAME:      [[ACCUM:%[0-9a-z]+]] = load i32, i32* %sat_a, align 4
----------------
This is wrong for the same reason; it shifts out the sign bit so the comparison could be different.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57553