Performance optimizations for base/numerics absolute value and multiply

Removes a number of branches from the calculations. Also now exporting
SafeUnsignedAbs as a public API.

NOTRY=true

Review-Url: https://codereview.chromium.org/2566733002
Cr-Commit-Position: refs/heads/master@{#437769}

base/numerics/safe_conversions.h

@@ -36,6 +36,9 @@ namespace base {
 //  IsValueNegative<>() - A convenience function that will accept any arithmetic
 //      type as an argument and will return whether the value is less than zero.
 //      Unsigned types always return false.
+//  SafeUnsignedAbs() - Returns the absolute value of the supplied integer
+//      parameter as an unsigned result (thus avoiding an overflow if the value
+//      is the signed, two's complement minimum).
 //  StrictNumeric<> - A wrapper type that performs assignments and copies via
 //      the strict_cast<> template, and can perform valid arithmetic comparisons
 //      across any range of arithmetic types. StrictNumeric is the return type
@@ -279,6 +282,7 @@ STRICT_COMPARISON_OP(IsNotEqual, !=);
 
 using internal::strict_cast;
 using internal::saturated_cast;
+using internal::SafeUnsignedAbs;
 using internal::StrictNumeric;
 using internal::MakeStrictNum;
 

base/numerics/safe_conversions_impl.h

@@ -30,6 +30,52 @@ struct IntegerBitsPlusSign {
                            std::is_signed<NumericType>::value;
 };
 
+// Helper templates for integer manipulations.
+
+template <typename Integer>
+struct PositionOfSignBit {
+  static const size_t value = IntegerBitsPlusSign<Integer>::value - 1;
+};
+
+template <typename T>
+constexpr bool HasSignBit(T x) {
+  // Cast to unsigned since right shift on signed is undefined.
+  return !!(static_cast<typename std::make_unsigned<T>::type>(x) >>
+            PositionOfSignBit<T>::value);
+}
+
+// This wrapper undoes the standard integer promotions.
+template <typename T>
+constexpr T BinaryComplement(T x) {
+  return static_cast<T>(~x);
+}
+
+// This performs a safe, non-branching absolute value via unsigned overflow.
+template <typename T>
+constexpr T SafeUnsignedAbsImpl(T value, T sign_mask) {
+  static_assert(!std::is_signed<T>::value, "Types must be unsigned.");
+  return (value + sign_mask) ^ sign_mask;
+}
+
+template <typename T,
+          typename std::enable_if<std::is_integral<T>::value &&
+                                  std::is_signed<T>::value>::type* = nullptr>
+constexpr typename std::make_unsigned<T>::type SafeUnsignedAbs(T value) {
+  using UnsignedT = typename std::make_unsigned<T>::type;
+  return SafeUnsignedAbsImpl(
+      static_cast<UnsignedT>(value),
+      // The sign mask is all ones for negative and zero otherwise.
+      static_cast<UnsignedT>(-static_cast<T>(HasSignBit(value))));
+}
+
+template <typename T,
+          typename std::enable_if<std::is_integral<T>::value &&
+                                  !std::is_signed<T>::value>::type* = nullptr>
+constexpr T SafeUnsignedAbs(T value) {
+  // T is unsigned, so |value| must already be positive.
+  return static_cast<T>(value);
+}
+
 enum IntegerRepresentation {
   INTEGER_REPRESENTATION_UNSIGNED,
   INTEGER_REPRESENTATION_SIGNED
@@ -288,11 +334,6 @@ struct TwiceWiderInteger {
                                        IsSigned>::type;
 };
 
-template <typename Integer>
-struct PositionOfSignBit {
-  static const size_t value = IntegerBitsPlusSign<Integer>::value - 1;
-};
-
 enum ArithmeticPromotionCategory {
   LEFT_PROMOTION,  // Use the type of the left-hand argument.
   RIGHT_PROMOTION  // Use the type of the right-hand argument.

base/numerics/safe_math_impl.h

@@ -42,21 +42,6 @@ struct UnsignedOrFloatForSize<Numeric, false, true> {
   using type = Numeric;
 };
 
-// Helper templates for integer manipulations.
-
-template <typename T>
-constexpr bool HasSignBit(T x) {
-  // Cast to unsigned since right shift on signed is undefined.
-  return !!(static_cast<typename std::make_unsigned<T>::type>(x) >>
-            PositionOfSignBit<T>::value);
-}
-
-// This wrapper undoes the standard integer promotions.
-template <typename T>
-constexpr T BinaryComplement(T x) {
-  return static_cast<T>(~x);
-}
-
 // Probe for builtin math overflow support on Clang and version check on GCC.
 #if defined(__has_builtin)
 #define USE_OVERFLOW_BUILTINS (__has_builtin(__builtin_add_overflow))
@@ -192,27 +177,21 @@ template <typename T,
               ((IntegerBitsPlusSign<T>::value * 2) >
                IntegerBitsPlusSign<intmax_t>::value)>::type* = nullptr>
 bool CheckedMulImpl(T x, T y, T* result) {
-  if (x && y) {
-    if (x > 0) {
-      if (y > 0) {
-        if (x > std::numeric_limits<T>::max() / y)
-          return false;
-      } else {
-        if (y < std::numeric_limits<T>::lowest() / x)
-          return false;
-      }
-    } else {
-      if (y > 0) {
-        if (x < std::numeric_limits<T>::lowest() / y)
-          return false;
-      } else {
-        if (y < std::numeric_limits<T>::max() / x)
-          return false;
-      }
-    }
-  }
-  *result = x * y;
-  return true;
+  // Since the value of x*y is potentially undefined if we have a signed type,
+  // we compute it using the unsigned type of the same size.
+  using UnsignedDst = typename std::make_unsigned<T>::type;
+  const T is_negative = HasSignBit(x) ^ HasSignBit(y);
+  const UnsignedDst ux = SafeUnsignedAbs(x);
+  const UnsignedDst uy = SafeUnsignedAbs(y);
+  UnsignedDst uresult = static_cast<UnsignedDst>(ux * uy);
+  // This is a non-branching conditional negation.
+  *result = static_cast<T>((uresult ^ -is_negative) + is_negative);
+  // This uses the unsigned overflow check on the absolute value, with a +1
+  // bound for a negative result.
+  return (uy == 0 ||
+          ux <= (static_cast<UnsignedDst>(std::numeric_limits<T>::max()) +
+                 is_negative) /
+                    uy);
 }
 
 template <typename T,
@@ -529,11 +508,8 @@ template <typename T,
           typename std::enable_if<std::is_integral<T>::value &&
                                   std::is_signed<T>::value>::type* = nullptr>
 bool CheckedAbs(T value, T* result) {
-  if (value != std::numeric_limits<T>::lowest()) {
-    *result = std::abs(value);
-    return true;
-  }
-  return false;
+  *result = static_cast<T>(SafeUnsignedAbs(value));
+  return *result != std::numeric_limits<T>::lowest();
 }
 
 template <typename T,
@@ -545,24 +521,6 @@ bool CheckedAbs(T value, T* result) {
   return true;
 }
 
-template <typename T,
-          typename std::enable_if<std::is_integral<T>::value &&
-                                  std::is_signed<T>::value>::type* = nullptr>
-constexpr typename std::make_unsigned<T>::type SafeUnsignedAbs(T value) {
-  using UnsignedT = typename std::make_unsigned<T>::type;
-  return value == std::numeric_limits<T>::lowest()
-             ? static_cast<UnsignedT>(std::numeric_limits<T>::max()) + 1
-             : static_cast<UnsignedT>(std::abs(value));
-}
-
-template <typename T,
-          typename std::enable_if<std::is_integral<T>::value &&
-                                  !std::is_signed<T>::value>::type* = nullptr>
-constexpr T SafeUnsignedAbs(T value) {
-  // T is unsigned, so |value| must already be positive.
-  return static_cast<T>(value);
-}
-
 // This is just boilerplate that wraps the standard floating point arithmetic.
 // A macro isn't the nicest solution, but it beats rewriting these repeatedly.
 #define BASE_FLOAT_ARITHMETIC_OPS(NAME, OP)                                    \

base/numerics/safe_numerics_unittest.cc

@@ -106,12 +106,15 @@ struct LogOnFailure {
 #define TEST_EXPECTED_SUCCESS(actual) TEST_EXPECTED_VALIDITY(true, actual)
 #define TEST_EXPECTED_FAILURE(actual) TEST_EXPECTED_VALIDITY(false, actual)
 
-#define TEST_EXPECTED_VALUE(expected, actual)                              \
-  EXPECT_EQ(static_cast<Dst>(expected),                                    \
-            ((actual)                                                      \
-                 .template Cast<Dst>()                                     \
-                 .template ValueOrDie<Dst, LogOnFailure>()))               \
-      << "Result test: Value " << GetNumericValueForTest(actual) << " as " \
+// We have to handle promotions, so infer the underlying type below from actual.
+#define TEST_EXPECTED_VALUE(expected, actual)                               \
+  EXPECT_EQ(static_cast<typename std::decay<decltype(actual)>::type::type>( \
+                expected),                                                  \
+            ((actual)                                                       \
+                 .template ValueOrDie<                                      \
+                     typename std::decay<decltype(actual)>::type::type,     \
+                     LogOnFailure>()))                                      \
+      << "Result test: Value " << GetNumericValueForTest(actual) << " as "  \
       << dst << " on line " << line
 
 // Test the simple pointer arithmetic overrides.
@@ -143,6 +146,8 @@ static void TestSpecializedArithmetic(
   TEST_EXPECTED_FAILURE(-CheckedNumeric<Dst>(DstLimits::lowest()));
   TEST_EXPECTED_FAILURE(CheckedNumeric<Dst>(DstLimits::lowest()).Abs());
   TEST_EXPECTED_VALUE(1, CheckedNumeric<Dst>(-1).Abs());
+  TEST_EXPECTED_VALUE(DstLimits::max(),
+                      MakeCheckedNum(-DstLimits::max()).Abs());
 
   TEST_EXPECTED_SUCCESS(CheckedNumeric<Dst>(DstLimits::max()) + -1);
   TEST_EXPECTED_FAILURE(CheckedNumeric<Dst>(DstLimits::lowest()) + -1);
@@ -161,6 +166,13 @@ static void TestSpecializedArithmetic(
   TEST_EXPECTED_FAILURE(CheckedNumeric<Dst>(DstLimits::lowest()) / -1);
   TEST_EXPECTED_VALUE(0, CheckedNumeric<Dst>(-1) / 2);
   TEST_EXPECTED_FAILURE(CheckedNumeric<Dst>(DstLimits::lowest()) * -1);
+  TEST_EXPECTED_VALUE(DstLimits::lowest(),
+                      MakeCheckedNum(DstLimits::lowest()).UnsignedAbs());
+  TEST_EXPECTED_VALUE(DstLimits::max(),
+                      MakeCheckedNum(DstLimits::max()).UnsignedAbs());
+  TEST_EXPECTED_VALUE(0, CheckedNumeric<Dst>(0).UnsignedAbs());
+  TEST_EXPECTED_VALUE(1, CheckedNumeric<Dst>(1).UnsignedAbs());
+  TEST_EXPECTED_VALUE(1, CheckedNumeric<Dst>(-1).UnsignedAbs());
 
   // Modulus is legal only for integers.
   TEST_EXPECTED_VALUE(0, CheckedNumeric<Dst>() % 1);
@@ -221,6 +233,12 @@ static void TestSpecializedArithmetic(
       CheckedNumeric<typename std::make_signed<Dst>::type>(
           std::numeric_limits<typename std::make_signed<Dst>::type>::lowest())
           .UnsignedAbs());
+  TEST_EXPECTED_VALUE(DstLimits::lowest(),
+                      MakeCheckedNum(DstLimits::lowest()).UnsignedAbs());
+  TEST_EXPECTED_VALUE(DstLimits::max(),
+                      MakeCheckedNum(DstLimits::max()).UnsignedAbs());
+  TEST_EXPECTED_VALUE(0, CheckedNumeric<Dst>(0).UnsignedAbs());
+  TEST_EXPECTED_VALUE(1, CheckedNumeric<Dst>(1).UnsignedAbs());
 
   // Modulus is legal only for integers.
   TEST_EXPECTED_VALUE(0, CheckedNumeric<Dst>() % 1);