[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+    CommonFixedSema.setSaturated(false);
+
----------------
ebevhan wrote:
> Can EmitFixedPointConversion not determine this on its own?
What I meant here was that rather than zeroing out the saturation mode on the common semantic because we know that the common conversion won't saturate, EmitFixedPointConversion should be able to pick up on the fact that converting from semantic A to semantic B shouldn't cause saturation and not emit a saturating conversion in that case. Then you can set the saturation on the common semantic properly, since the operation should be saturating.

Even for something like `sat_uf * sat_uf` with padding, where the common type conversion should be `(16w, 15scale, uns, sat, pad) -> (15w, 15scale, uns, sat, nopad)`, EmitFixedPointConversion shouldn't emit a saturation, since the number of integral bits hasn't changed.


================
Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 [[USA_TRUNC]], i15 [[USA_SAT_TRUNC]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2
----------------
This is probably a candidate for an isel optimization. This operation also works as an `i16 ssat.add` with a negative-clamp-to-zero afterwards, and if the target supports `i16 ssat.add` natively then it will likely be a lot more efficient than whatever an `i15 uadd.sat` produces.


Repository:
  rC Clang

https://reviews.llvm.org/D53738