Skip to content

Commit

Permalink
Revert "Reorder saturation calculation for ClampAddOp and ClampSupOp"
Browse files Browse the repository at this point in the history
This reverts commit ffc71c0.

Reason for revert: Didn't improve the perf issue.

Original change's description:
> Reorder saturation calculation for ClampAddOp and ClampSupOp
> 
> The order of operations changed in crrev.com/505494 with C++14
> constexpr optimizations. However, that change appears to have regressed
> performance in some cases. So, this CL tweaks the code to match the
> old structure and trigger the same compiler optimization heuristics.
> 
> Bug: 770832
> Change-Id: I28f57ddf53825619eb73c9a92c663fc8463e16a7
> Reviewed-on: https://chromium-review.googlesource.com/696721
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Commit-Queue: Justin Schuh <jschuh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#506078}

TBR=jschuh@chromium.org,tsepez@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 770832
Change-Id: I076c93215b422e321ad4bf39a72bab144cc0195d
Reviewed-on: https://chromium-review.googlesource.com/700775
Reviewed-by: Justin Schuh <jschuh@chromium.org>
Commit-Queue: Justin Schuh <jschuh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506559}
  • Loading branch information
jschuh authored and Commit Bot committed Oct 4, 2017
1 parent 055fe8f commit 95a989c
Showing 1 changed file with 38 additions and 36 deletions.
74 changes: 38 additions & 36 deletions base/numerics/clamped_math_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,30 +82,31 @@ struct ClampedAddOp<T,
if (ClampedAddFastOp<T, U>::is_supported)
return ClampedAddFastOp<T, U>::template Do<V>(x, y);

// This is structured as a nested ternary to trigger the necessary
// optimization heuristics in MSVC and Clang.
const V saturated =
IsTypeInRangeForNumericType<V, T>::value
? (IsTypeInRangeForNumericType<V, U>::value
// Optimize for a compile-time constant in the common case.
? CommonMaxOrMin<V>(
(IsCompileTimeConstant(x) && IsValueNegative(x)) ||
IsValueNegative(y))
// Otherwise the out-of-range-type determines the saturation.
: CommonMaxOrMin<V>(IsValueNegative(y)))
: CommonMaxOrMin<V>(IsValueNegative(x));

// Pick a destination type wide enough to compute the saturation direction.
// We already computed the saturation value, so it's structured this way to
// avoid run-time conversions when we have a compile-time-constant.
// The saturation check in the final return statement covers the case where
// one type is out-of-bounds. It's structured this way to avoid unnecessary
// run-time conversions when we have a compile-time-constant.
using Promotion = typename std::conditional_t<
IsTypeInRangeForNumericType<V, T>::value ||
IsTypeInRangeForNumericType<V, U>::value,
V, typename BigEnoughPromotion<T, U>::type>;
Promotion result;
return BASE_NUMERICS_LIKELY((CheckedAddOp<T, U>::Do(x, y, &result)))
? saturated_cast<V>(result)
: saturated;
if (BASE_NUMERICS_LIKELY((CheckedAddOp<T, U>::Do(x, y, &result))))
return saturated_cast<V>(result);

// This is the normal saturation case, which includes a compile-time
// constant optimization.
if (IsTypeInRangeForNumericType<V, T>::value &&
IsTypeInRangeForNumericType<V, U>::value) {
return CommonMaxOrMin<V>(
(IsCompileTimeConstant(x) && IsValueNegative(x)) ||
IsValueNegative(y));
}

// Otherwise the out-of-range-type determines the saturation direction.
return IsTypeInRangeForNumericType<V, T>::value
? CommonMaxOrMin<V>(IsValueNegative(y))
: CommonMaxOrMin<V>(IsValueNegative(x));
}
};

Expand All @@ -124,30 +125,31 @@ struct ClampedSubOp<T,
if (ClampedSubFastOp<T, U>::is_supported)
return ClampedSubFastOp<T, U>::template Do<V>(x, y);

// This is structured as a nested ternary to trigger the necessary
// optimization heuristics in MSVC and Clang.
const V saturated =
IsTypeInRangeForNumericType<V, T>::value
? (IsTypeInRangeForNumericType<V, U>::value
// Optimize for a compile-time constant in the common case.
? CommonMaxOrMin<V>(
(IsCompileTimeConstant(x) && IsValueNegative(x)) ||
!IsValueNegative(y))
// Otherwise the out-of-range-type determines the saturation.
: CommonMaxOrMin<V>(!IsValueNegative(y)))
: CommonMaxOrMin<V>(IsValueNegative(x));

// Pick a destination type wide enough to compute the saturation direction.
// We already computed the saturation value, so it's structured this way to
// avoid run-time conversions when we have a compile-time-constant.
// The saturation check in the final return statement covers the case where
// one type is out-of-bounds. It's structured this way to avoid unnecessary
// run-time conversions when we have a compile-time-constant.
using Promotion = typename std::conditional_t<
IsTypeInRangeForNumericType<V, T>::value ||
IsTypeInRangeForNumericType<V, U>::value,
V, typename BigEnoughPromotion<T, U>::type>;
Promotion result;
return BASE_NUMERICS_LIKELY((CheckedSubOp<T, U>::Do(x, y, &result)))
? saturated_cast<V>(result)
: saturated;
if (BASE_NUMERICS_LIKELY((CheckedSubOp<T, U>::Do(x, y, &result))))
return saturated_cast<V>(result);

// This is the normal saturation case, which includes a compile-time
// constant optimization.
if (IsTypeInRangeForNumericType<V, T>::value &&
IsTypeInRangeForNumericType<V, U>::value) {
return CommonMaxOrMin<V>(
(IsCompileTimeConstant(x) && IsValueNegative(x)) ||
!IsValueNegative(y));
}

// Otherwise the out-of-range-type determines the saturation direction.
return IsTypeInRangeForNumericType<V, T>::value
? CommonMaxOrMin<V>(!IsValueNegative(y))
: CommonMaxOrMin<V>(IsValueNegative(x));
}
};

Expand Down

0 comments on commit 95a989c

Please sign in to comment.