[llvm] r253921 - [Support] Add optional argument to SaturatingAdd() and SaturatingMultiply() to indicate that overflow occurred

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 24 16:39:05 PST 2015


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


More information about the llvm-commits mailing list