This is an archive of the discontinued LLVM Phabricator instance.

[Intrinsic] Unigned Saturation Addition Intrinsic
ClosedPublic

Authored by leonardchan on Oct 16 2018, 2:09 PM.

Details

Summary

Add an intrinsic that takes 2 integers and perform unsigned saturation addition on them.

This is a part of implementing fixed point arithmetic in clang where some of the more complex operations will be implemented as intrinsics.

Diff Detail

Repository
rL LLVM

Event Timeline

leonardchan created this revision.Oct 16 2018, 2:09 PM

Is this intrinsic really needed for the fixed point support?

Maybe I've missed something in the discussions about support for unsigned fixed point types, but in our implementation the unsigned types are using the positive range of the signed type, such as [0, SIGNED_MAX]. So we keep the sign bit zero for the unsigned fixed point types. Are you planning for something different here?

In our implementation we lower addition of two saturated unsigned fixed point types to sadd.sat (if the input values are in the range [0, SIGNED_MAX] the result will be in the range [0, SIGNED_MAX] as well). So we haven't really found a need for implementing uadd.sat.

LGTM!

In our implementation we lower addition of two saturated unsigned fixed point types to sadd.sat (if the input values are in the range [0, SIGNED_MAX] the result will be in the range [0, SIGNED_MAX] as well). So we haven't really found a need for implementing uadd.sat.

Our implementation is one without the padding bit on unsigned types, so we don't need a uadd.sat. The same applies to fixumul (which we also don't have). As you say, unsigned operations can be implemented in terms of the signed ones in that kind of implementation.

bjope added inline comments.Oct 17 2018, 1:56 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
4685

Sum >= 0

Also, these comments should probably be moved inside the if clause below (as they apply for SADDSAT).
For UADDSAT it is enough to check if we got overflow.

nhaehnle removed a subscriber: nhaehnle.Oct 17 2018, 3:13 AM
leonardchan marked an inline comment as done.

Is this intrinsic really needed for the fixed point support?

Maybe I've missed something in the discussions about support for unsigned fixed point types, but in our implementation the unsigned types are using the positive range of the signed type, such as [0, SIGNED_MAX]. So we keep the sign bit zero for the unsigned fixed point types. Are you planning for something different here?

In our implementation we lower addition of two saturated unsigned fixed point types to sadd.sat (if the input values are in the range [0, SIGNED_MAX] the result will be in the range [0, SIGNED_MAX] as well). So we haven't really found a need for implementing uadd.sat.

By default we have unsigned fixed point types in clang to use the entire length of the int, so it ranges from [0, UNSIGNED_MAX]. In this case I think it would be useful to have an unsigned version as well. This could also be used for regular unsigned integers that do not have a padding bit, and I believe @craig.topper was interested in this so they could be replaced with target specific calls to x86 intrinsics like PADDUSB and PADDUSW.

bjope added a comment.Oct 17 2018, 9:41 AM

Is this intrinsic really needed for the fixed point support?

Maybe I've missed something in the discussions about support for unsigned fixed point types, but in our implementation the unsigned types are using the positive range of the signed type, such as [0, SIGNED_MAX]. So we keep the sign bit zero for the unsigned fixed point types. Are you planning for something different here?

In our implementation we lower addition of two saturated unsigned fixed point types to sadd.sat (if the input values are in the range [0, SIGNED_MAX] the result will be in the range [0, SIGNED_MAX] as well). So we haven't really found a need for implementing uadd.sat.

By default we have unsigned fixed point types in clang to use the entire length of the int, so it ranges from [0, UNSIGNED_MAX]. In this case I think it would be useful to have an unsigned version as well. This could also be used for regular unsigned integers that do not have a padding bit, and I believe @craig.topper was interested in this so they could be replaced with target specific calls to x86 intrinsics like PADDUSB and PADDUSW.

Ok, I see. That explains the need for it.

I haven't really reviewed the test case in detail (I'm not that familiar with the X86 vector instructions), but this LGTM.

@craig.topper Any more comments on this patch?

This revision is now accepted and ready to land.Oct 22 2018, 3:36 PM
This revision was automatically updated to reflect the committed changes.