[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
Tue Nov 24 17:09:44 PST 2015
Could it expose a problem of Log2_64 on BE systems?
David
On Tue, Nov 24, 2015 at 4:39 PM, Hal Finkel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Hi Nathan,
>
> FYI: These tests are failing on my big-Endian PowerPC (P7) system
> (compiled with GCC 4.8.2). I see no problems with a self-hosted build (and
> looking over the commit, I can see nothing wrong with it either).
>
> I don't think it is worth reverting over a PPC64/4.8.2 miscompile, but
> I'll let you know when I try with a newer GCC version.
>
> Note: Google Test filter = MathExtras.SaturatingMultiply
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from MathExtras
> [ RUN ] MathExtras.SaturatingMultiply
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 98303
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 98303
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 245759
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 245759
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 516095
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 516095
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 1044479
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 1044479
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 2095103
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 2095103
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 4193279
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 4193279
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 8388095
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 8388095
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 16776959
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 16776959
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 33554303
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 33554303
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 67108799
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 67108799
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 134217695
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 134217695
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 268435439
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 268435439
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 536870903
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 536870903
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 1073741819
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 1073741819
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> /src/llvm/unittests/Support/MathExtrasTest.cpp:288: Failure
> Value of: SaturatingMultiply(X, Y)
> Actual: 2147483645
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:289: Failure
> Value of: SaturatingMultiply(X, Y, ResultOverflowed)
> Actual: 2147483645
> Expected: Max
> Which is: 4294967295
> /src/llvm/unittests/Support/MathExtrasTest.cpp:290: Failure
> Value of: ResultOverflowed
> Actual: false
> Expected: true
> [ FAILED ] MathExtras.SaturatingMultiply (8 ms)
> [----------] 1 test from MathExtras (8 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (8 ms total)
> [ PASSED ] 0 tests.
> [ FAILED ] 1 test, listed below:
> [ FAILED ] MathExtras.SaturatingMultiply
>
> 1 FAILED TEST
>
> ********************
> Testing Time: 111.43s
> ********************
> Failing Tests (1):
> LLVM-Unit ::
> Support/Release+Asserts/SupportTests/MathExtras.SaturatingMultiply
>
> -Hal
>
> ----- Original Message -----
> > From: "Nathan Slingerland via llvm-commits" <llvm-commits at lists.llvm.org
> >
> > To: llvm-commits at lists.llvm.org
> > Sent: Monday, November 23, 2015 3:54:23 PM
> > Subject: [llvm] r253921 - [Support] Add optional argument to
> SaturatingAdd() and SaturatingMultiply() to indicate
> > that overflow occurred
> >
> > 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
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> 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/20151124/2e54cbe7/attachment.html>
More information about the llvm-commits
mailing list