[llvm] r253921 - [Support] Add optional argument to SaturatingAdd() and SaturatingMultiply() to indicate that overflow occurred
Nathan Slingerland via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 3 15:38:26 PST 2015
After thinking about this a bit more I can see two benefits to Sean's
approach:
1) a pointer better advertises at the call site that the argument will be
modified
2) the implementation is smaller for optional arguments - once we add a
fused saturating multiply-add support function that means 6 vs 3 functions
Finally, it does appear that using a pointer in this situation is more
inline with common practice. For example, Google's C++ style guide:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments
"...it is a very strong convention in Google code that input arguments are
values or const references while output arguments are pointers."
On Mon, Nov 23, 2015 at 3:48 PM, Xinliang David Li <xinliangli at gmail.com>
wrote:
> 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/20151203/2416364d/attachment.html>
More information about the llvm-commits
mailing list