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

[Leonard Chan via Phabricator via cfe-commits]

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

In D53738#1309171 <https://reviews.llvm.org/D53738#1309171>, @ebevhan wrote:

> In D53738#1308314 <https://reviews.llvm.org/D53738#1308314>, @leonardchan wrote:
>
> > > 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.
>
>
> They don't? The example given by the spec is even `int * _Fract`.


Oh, I meant that the scales of both operands won't need to be aligned before performing the operation. Since for multiplication, we can multiply fixed point numbers in any scale without shifting and only need to perform a shift on the result to convert to the destination type. Although this would only apply to non-saturating multiplication since to use the intrinsics, both operands would need to be in the same scale.

@rjmccall Any more comments on this patch?



================
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:
> > > 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.
> No, it doesn't have to be changed. Just something to keep in mind.
> > i15 ssat.add would be legalized into i16 ssat.add if that is what's natively supported.
> Sure, but I meant that `i15 usat.add` could be more efficient as `i16 ssat.add`.
Got it. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738