[llvm] r253921 - [Support] Add optional argument to SaturatingAdd() and SaturatingMultiply() to indicate that overflow occurred
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 23 15:48:41 PST 2015
This looks nice -- but IMO with the pattern, both solutions now look
equally good to me :)
David
On Mon, Nov 23, 2015 at 3:41 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Mon, Nov 23, 2015 at 3:31 PM, Nathan Slingerland <slingn at gmail.com>
> wrote:
>
>> Right - I tried it both ways. Using a pointer made the
>> SaturatingMultiply() code a bit harder to read since it needs to check for
>> non-null at each point where `ResultOverflowed` should be updated.
>> Otherwise the two ways seem equivalent.
>>
>
> This kind of pattern avoids it:
>
> bool Dummy;
> bool &Overflowed = ResultOverflowed ? ResultOverflowed : &Dummy;
>
> or using a lambda:
>
> auto setOverflowed = [&]() { if (ResultOverflowed) *ResultOverflowed =
> true; };
>
> Either pattern seems preferable to duplicating the template/enable_if
> stuff.
>
> -- Sean Silva
>
>
>>
>>
>> On Mon, Nov 23, 2015 at 3:30 PM, Xinliang David Li <xinliangli at gmail.com>
>> wrote:
>>
>>> One reason I can see is that using default arguments will resulting in
>>> bigger code -- as all the updates need to check nullness.
>>>
>>> David
>>>
>>> On Mon, Nov 23, 2015 at 3:20 PM, Sean Silva via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Is there a reason you used overloading instead of default arguments?
>>>> I.e. instead of just declaring the ResultOverflowed variable as `bool
>>>> *ResultOverflowed = nullptr`.
>>>>
>>>> -- Sean Silva
>>>>
>>>> On Mon, Nov 23, 2015 at 1:54 PM, Nathan Slingerland via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: slingn
>>>>> Date: Mon Nov 23 15:54:22 2015
>>>>> New Revision: 253921
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=253921&view=rev
>>>>> Log:
>>>>> [Support] Add optional argument to SaturatingAdd() and
>>>>> SaturatingMultiply() to indicate that overflow occurred
>>>>>
>>>>> Summary: Adds the ability for callers to detect when saturation
>>>>> occurred on the result of saturating addition/multiplication.
>>>>>
>>>>> Reviewers: davidxl, silvas, rsmith
>>>>>
>>>>> Subscribers: llvm-commits
>>>>>
>>>>> Differential Revision: http://reviews.llvm.org/D14931
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/include/llvm/Support/MathExtras.h
>>>>> llvm/trunk/unittests/Support/MathExtrasTest.cpp
>>>>>
>>>>> Modified: llvm/trunk/include/llvm/Support/MathExtras.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253921&r1=253920&r2=253921&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/Support/MathExtras.h (original)
>>>>> +++ llvm/trunk/include/llvm/Support/MathExtras.h Mon Nov 23 15:54:22
>>>>> 2015
>>>>> @@ -655,46 +655,79 @@ inline int64_t SignExtend64(uint64_t X,
>>>>>
>>>>> /// \brief Add two unsigned integers, X and Y, of type T.
>>>>> /// Clamp the result to the maximum representable value of T on
>>>>> overflow.
>>>>> +/// ResultOverflowed indicates if the result is larger than the
>>>>> maximum
>>>>> +/// representable value of type T.
>>>>> template <typename T>
>>>>> typename std::enable_if<std::is_unsigned<T>::value, T>::type
>>>>> -SaturatingAdd(T X, T Y) {
>>>>> +SaturatingAdd(T X, T Y, bool &ResultOverflowed) {
>>>>> // Hacker's Delight, p. 29
>>>>> T Z = X + Y;
>>>>> - if (Z < X || Z < Y)
>>>>> + ResultOverflowed = (Z < X || Z < Y);
>>>>> + if (ResultOverflowed)
>>>>> return std::numeric_limits<T>::max();
>>>>> else
>>>>> return Z;
>>>>> }
>>>>>
>>>>> +/// \brief Add two unsigned integers, X and Y, of type T.
>>>>> +/// Clamp the result to the maximum representable value of T on
>>>>> overflow.
>>>>> +template <typename T>
>>>>> +typename std::enable_if<std::is_unsigned<T>::value, T>::type
>>>>> +SaturatingAdd(T X, T Y) {
>>>>> + bool ResultOverflowed;
>>>>> + return SaturatingAdd(X, Y, ResultOverflowed);
>>>>> +}
>>>>> +
>>>>> /// \brief Multiply two unsigned integers, X and Y, of type T.
>>>>> /// Clamp the result to the maximum representable value of T on
>>>>> overflow.
>>>>> +/// ResultOverflowed indicates if the result is larger than the
>>>>> maximum
>>>>> +/// representable value of type T.
>>>>> template <typename T>
>>>>> typename std::enable_if<std::is_unsigned<T>::value, T>::type
>>>>> -SaturatingMultiply(T X, T Y) {
>>>>> +SaturatingMultiply(T X, T Y, bool &ResultOverflowed) {
>>>>> // Hacker's Delight, p. 30 has a different algorithm, but we don't
>>>>> use that
>>>>> // because it fails for uint16_t (where multiplication can have
>>>>> undefined
>>>>> // behavior due to promotion to int), and requires a division in
>>>>> addition
>>>>> // to the multiplication.
>>>>>
>>>>> + ResultOverflowed = false;
>>>>> +
>>>>> // Log2(Z) would be either Log2Z or Log2Z + 1.
>>>>> // Special case: if X or Y is 0, Log2_64 gives -1, and Log2Z
>>>>> // will necessarily be less than Log2Max as desired.
>>>>> int Log2Z = Log2_64(X) + Log2_64(Y);
>>>>> const T Max = std::numeric_limits<T>::max();
>>>>> int Log2Max = Log2_64(Max);
>>>>> - if (Log2Z < Log2Max)
>>>>> + if (Log2Z < Log2Max) {
>>>>> return X * Y;
>>>>> - if (Log2Z > Log2Max)
>>>>> + }
>>>>> + if (Log2Z > Log2Max) {
>>>>> + ResultOverflowed = true;
>>>>> return Max;
>>>>> + }
>>>>>
>>>>> // We're going to use the top bit, and maybe overflow one
>>>>> // bit past it. Multiply all but the bottom bit then add
>>>>> // that on at the end.
>>>>> T Z = (X >> 1) * Y;
>>>>> - if (Z & ~(Max >> 1))
>>>>> + if (Z & ~(Max >> 1)) {
>>>>> + ResultOverflowed = true;
>>>>> return Max;
>>>>> + }
>>>>> Z <<= 1;
>>>>> - return (X & 1) ? SaturatingAdd(Z, Y) : Z;
>>>>> + if (X & 1)
>>>>> + return SaturatingAdd(Z, Y, ResultOverflowed);
>>>>> +
>>>>> + return Z;
>>>>> +}
>>>>> +
>>>>> +/// \brief Multiply two unsigned integers, X and Y, of type T.
>>>>> +/// Clamp the result to the maximum representable value of T on
>>>>> overflow.
>>>>> +template <typename T>
>>>>> +typename std::enable_if<std::is_unsigned<T>::value, T>::type
>>>>> +SaturatingMultiply(T X, T Y) {
>>>>> + bool ResultOverflowed;
>>>>> + return SaturatingMultiply(X, Y, ResultOverflowed);
>>>>> }
>>>>>
>>>>> extern const float huge_valf;
>>>>>
>>>>> Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253921&r1=253920&r2=253921&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)
>>>>> +++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Mon Nov 23
>>>>> 15:54:22 2015
>>>>> @@ -194,10 +194,27 @@ template<typename T>
>>>>> void SaturatingAddTestHelper()
>>>>> {
>>>>> const T Max = std::numeric_limits<T>::max();
>>>>> + bool ResultOverflowed;
>>>>> +
>>>>> EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2)));
>>>>> + EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(Max, SaturatingAdd(Max, T(1)));
>>>>> + EXPECT_EQ(Max, SaturatingAdd(Max, T(1), ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> +
>>>>> + EXPECT_EQ(Max, SaturatingAdd(T(1), T(Max - 1)));
>>>>> + EXPECT_EQ(Max, SaturatingAdd(T(1), T(Max - 1), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(Max, SaturatingAdd(T(1), Max));
>>>>> + EXPECT_EQ(Max, SaturatingAdd(T(1), Max, ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(Max, SaturatingAdd(Max, Max));
>>>>> + EXPECT_EQ(Max, SaturatingAdd(Max, Max, ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> }
>>>>>
>>>>> TEST(MathExtras, SaturatingAdd) {
>>>>> @@ -211,22 +228,50 @@ template<typename T>
>>>>> void SaturatingMultiplyTestHelper()
>>>>> {
>>>>> const T Max = std::numeric_limits<T>::max();
>>>>> + bool ResultOverflowed;
>>>>>
>>>>> // Test basic multiplication.
>>>>> EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3)));
>>>>> + EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(T(6), SaturatingMultiply(T(3), T(2)));
>>>>> + EXPECT_EQ(T(6), SaturatingMultiply(T(3), T(2), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>>
>>>>> // Test multiplication by zero.
>>>>> EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(0)));
>>>>> + EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(0), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0)));
>>>>> + EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1)));
>>>>> + EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(T(0), SaturatingMultiply(Max, T(0)));
>>>>> + EXPECT_EQ(T(0), SaturatingMultiply(Max, T(0), ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(T(0), SaturatingMultiply(T(0), Max));
>>>>> + EXPECT_EQ(T(0), SaturatingMultiply(T(0), Max, ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>>
>>>>> // Test multiplication by maximum value.
>>>>> EXPECT_EQ(Max, SaturatingMultiply(Max, T(2)));
>>>>> - EXPECT_EQ(Max, SaturatingMultiply(T(2),Max));
>>>>> + EXPECT_EQ(Max, SaturatingMultiply(Max, T(2), ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> +
>>>>> + EXPECT_EQ(Max, SaturatingMultiply(T(2), Max));
>>>>> + EXPECT_EQ(Max, SaturatingMultiply(T(2), Max, ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> +
>>>>> EXPECT_EQ(Max, SaturatingMultiply(Max, Max));
>>>>> + EXPECT_EQ(Max, SaturatingMultiply(Max, Max, ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>>
>>>>> // Test interesting boundary conditions for algorithm -
>>>>> // ((1 << A) - 1) * ((1 << B) + K) for K in [-1, 0, 1]
>>>>> @@ -241,8 +286,12 @@ void SaturatingMultiplyTestHelper()
>>>>>
>>>>> if(OverflowExpected) {
>>>>> EXPECT_EQ(Max, SaturatingMultiply(X, Y));
>>>>> + EXPECT_EQ(Max, SaturatingMultiply(X, Y, ResultOverflowed));
>>>>> + EXPECT_TRUE(ResultOverflowed);
>>>>> } else {
>>>>> EXPECT_EQ(X * Y, SaturatingMultiply(X, Y));
>>>>> + EXPECT_EQ(X * Y, SaturatingMultiply(X, Y, ResultOverflowed));
>>>>> + EXPECT_FALSE(ResultOverflowed);
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151123/c75c526b/attachment.html>
More information about the llvm-commits
mailing list