ebevhan added a comment. 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`. ================ 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 ---------------- 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`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53738/new/ https://reviews.llvm.org/D53738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
