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

Mon Nov 26 10:39:19 PST 2018

leonardchan marked an inline comment as done.
leonardchan added a comment.

> Generally I think it's good! One final note; I assume we could technically reuse/rename the EmitFixedPointAdd function and use it to emit other binops when those are added?

Yes, but I imagine if we choose to keep the call to `EmitFixedPointConversion` to cast both operands to a common type, this wouldn't be reused for division or multiplication since I believe those do not require for the operands to be converted to a common type.

================
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
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > 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.
> > Do you think it would be more efficient for now then if instead we did SHL by 1, saturate, then [AL]SHR by 1? This way we could use `i16 ssat.add` instead of `i15 ssat.add`?
> We should probably just do it in isel or instcombine instead. We don't know at this point which intrinsic is a better choice (though, I think power-of-two-types are generally better).
Ok. Are you suggesting something should be changed here though? I imagine that during legalization, `i15 ssat.add` would be legalized into `i16 ssat.add` if that is what's natively supported.

Repository:
  rC Clang

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

https://reviews.llvm.org/D53738