[llvm] r253921 - [Support] Add optional argument to SaturatingAdd() and SaturatingMultiply() to indicate that overflow occurred
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 3 17:25:28 PST 2015
On Thu, Dec 3, 2015 at 3:38 PM, Nathan Slingerland <slingn at gmail.com> wrote:
> 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."
>
Just FYI but LLVM style definitely deviates from this convention
significantly (but for default arguments like this I think we do favor
using pointers, since "nullness" is convenient to express).
-- Sean Silva
>
>
> 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/bf4cee6b/attachment.html>
More information about the llvm-commits
mailing list