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

[leonardchan]

Summary

When both the source and target types have the same width, avoid resizing. This gets rid of up to 2 extra instructions when converting to a saturation type.

[ebevhan]

This doesn't seem to address the particular case in the integer conversion patch. In fact, I don't see those conversions at all.

clang/test/Frontend/fixed_point_add.c

382 ↗

(On Diff #184641)

Hm, there's a weakness with this emission. This is no longer a min-max pattern, which means it will be a lot harder to generate code for.

[leonardchan]

In D57553#1380157, @ebevhan wrote:

This doesn't seem to address the particular case in the integer conversion patch. In fact, I don't see those conversions at all.

I was initially planning on rebasing and updating the parent patch of this after you took a look at it, but I did not account for the min-max pattern. Incidentally, we can still get the same results without having to change a lot of other tests or existing code if we just add a check for if the source and target have the same width. The saturation test cases these affect are the same as in the previous revision without having to change the other operations and still uses the min-max pattern.

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.

[ebevhan]

In 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.

clang/test/Frontend/fixed_point_conversions.c

194

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.

222

This is wrong for the same reason; it shifts out the sign bit so the comparison could be different.

[leonardchan]

In D57553#1388493, @ebevhan wrote:

In 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.

No problem. Abandoning this patch.

clang/test/Frontend/fixed_point_conversions.c

194

Ah you're right. I completely missed this.

clang/lib/CodeGen/CGExprScalar.cpp

if (!DstFPSema.isSaturated()) {
 
     // Upscale.
     if (DstScale > SrcScale)
       Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
   } else {
     // Adjust the number of fractional bits.
     if (DstScale > SrcScale) {
       // Compare to DstWidth to prevent resizing twice.
+      if (ResultWidth != DstWidth) {
+        // No rescaling is needed if both sides have the same resulting width.
+        // This would end up with a needless int cast.
         ResultWidth = std::max(SrcWidth + DstScale - SrcScale, DstWidth);
         llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);
-      Result = Builder.CreateIntCast(Result, UpscaledTy, SrcIsSigned, "resize");
+        Result =
+            Builder.CreateIntCast(Result, UpscaledTy, SrcIsSigned, "resize");
+      }
       Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
     } else if (DstScale < SrcScale) {
       Result = SrcIsSigned ?
           Builder.CreateAShr(Result, SrcScale - DstScale, "downscale") :
           Builder.CreateLShr(Result, SrcScale - DstScale, "downscale");
     }
 
     // Handle saturation.

clang/test/Frontend/fixed_point_conversions.c

void TestFixedPointCastSaturation() {
   // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[RESULT]], -32768
   // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 -32768, i32 [[RESULT]]
   // DEFAULT-NEXT: [[RESULT_TRUNC:%[0-9a-z]+]] = trunc i32 [[RESULT2]] to i16
   // DEFAULT-NEXT: store i16 [[RESULT_TRUNC]], i16* %sat_f, align 2
 
   // Accum to Fract, increasing scale
   sat_lf = sat_a;
   // DEFAULT:      [[OLD_ACCUM:%[0-9a-z]+]] = load i32, i32* %sat_a, align 4
-  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = sext i32 [[OLD_ACCUM]] to i48
-  // DEFAULT-NEXT: [[FRACT:%[0-9a-z]+]] = shl i48 [[ACCUM]], 16
-  // DEFAULT-NEXT: [[USE_MAX:%[0-9a-z]+]] = icmp sgt i48 [[FRACT]], 2147483647
-  // DEFAULT-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[USE_MAX]], i48 2147483647, i48 [[FRACT]]
-  // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i48 [[RESULT]], -2147483648
-  // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i48 -2147483648, i48 [[RESULT]]
-  // DEFAULT-NEXT: [[RESULT_TRUNC:%[0-9a-z]+]] = trunc i48 [[RESULT2]] to i32
-  // DEFAULT-NEXT: store i32 [[RESULT_TRUNC]], i32* %sat_lf, align 4
+  // DEFAULT-NEXT: [[FRACT:%[0-9a-z]+]] = shl i32 [[OLD_ACCUM]], 16
+  // DEFAULT-NEXT: [[USE_MAX:%[0-9a-z]+]] = icmp sgt i32 [[FRACT]], 2147483647
+  // DEFAULT-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[USE_MAX]], i32 2147483647, i32 [[FRACT]]
+  // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[RESULT]], -2147483648
+  // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 -2147483648, i32 [[RESULT]]
+  // DEFAULT-NEXT: store i32 [[RESULT2]], i32* %sat_lf, align 4
 
   // Signed to unsigned, decreasing scale
   _Sat _Accum sat_a2;
   sat_usa = sat_a2;
   // DEFAULT:      [[OLD_ACCUM:%[0-9a-z]+]] = load i32, i32* %sat_a2, align 4
   // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = ashr i32 [[OLD_ACCUM]], 7
   // DEFAULT-NEXT: [[USE_MAX:%[0-9a-z]+]] = icmp sgt i32 [[ACCUM]], 65535
   // DEFAULT-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[USE_MAX]], i32 65535, i32 [[ACCUM]]
   // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[RESULT]], 0
   // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 0, i32 [[RESULT]]
   // DEFAULT-NEXT: [[RESULT_TRUNC:%[0-9a-z]+]] = trunc i32 [[RESULT2]] to i16
   // DEFAULT-NEXT: store i16 [[RESULT_TRUNC]], i16* %sat_usa, align 2
   // SAME:      [[OLD_ACCUM:%[0-9a-z]+]] = load i32, i32* %sat_a2, align 4
   // SAME-NEXT: [[ACCUM:%[0-9a-z]+]] = ashr i32 [[OLD_ACCUM]], 8
   // SAME-NEXT: [[USE_MAX:%[0-9a-z]+]] = icmp sgt i32 [[ACCUM]], 32767
   // SAME-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[USE_MAX]], i32 32767, i32 [[ACCUM]]
   // SAME-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[RESULT]], 0
   // SAME-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 0, i32 [[RESULT]]
   // SAME-NEXT: [[RESULT_TRUNC:%[0-9a-z]+]] = trunc i32 [[RESULT2]] to i16
   // SAME-NEXT: store i16 [[RESULT_TRUNC]], i16* %sat_usa, align 2
 
   // Signed to unsigned, increasing scale
   sat_ua = sat_a;
   // DEFAULT:      [[OLD_ACCUM:%[0-9a-z]+]] = load i32, i32* %sat_a, align 4
-  // DEFAULT-NEXT: [[ACCUM_EXT:%[0-9a-z]+]] = sext i32 [[OLD_ACCUM]] to i33
-  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i33 [[ACCUM_EXT]], 1
-  // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i33 [[ACCUM]], 0
-  // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i33 0, i33 [[ACCUM]]
-  // DEFAULT-NEXT: [[RESULT_TRUNC:%[0-9a-z]+]] = trunc i33 [[RESULT2]] to i32
-  // DEFAULT-NEXT: store i32 [[RESULT_TRUNC]], i32* %sat_ua, align 4
+  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i32 [[OLD_ACCUM]], 1
+  // DEFAULT-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[ACCUM]], 0
+  // 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
   // SAME-NEXT: [[USE_MIN:%[0-9a-z]+]] = icmp slt i32 [[ACCUM]], 0
   // SAME-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 0, i32 [[ACCUM]]
   // SAME-NEXT: store i32 [[RESULT]], i32* %sat_ua, align 4
 
   // Nothing when saturating to the same type and size
   sat_a = a;
   // DEFAULT:      [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4