<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 23, 2015 at 3:31 PM, Nathan Slingerland <span dir="ltr"><<a href="mailto:slingn@gmail.com" target="_blank">slingn@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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.<br></div></blockquote><div><br></div><div><div>This kind of pattern avoids it:</div><div><br></div><div>bool Dummy;</div><div>bool &Overflowed = ResultOverflowed ? ResultOverflowed : &Dummy;</div><div><br></div><div>or using a lambda:</div><div><br></div><div>auto setOverflowed = [&]() { if (ResultOverflowed) *ResultOverflowed = true; };</div><div><br></div><div>Either pattern seems preferable to duplicating the template/<span style="font-size:13px">enable_if</span> stuff.</div><div><br></div><div>-- Sean Silva</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br></div><div class=""><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 23, 2015 at 3:30 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">One reason I can see is that using default arguments will resulting in bigger code -- as all the updates need to check nullness.<span><font color="#888888"><div><br></div><div>David</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 23, 2015 at 3:20 PM, Sean Silva via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Is there a reason you used overloading instead of default arguments? I.e. instead of just declaring the ResultOverflowed variable as `bool *ResultOverflowed = nullptr`.<br><div><br></div><div>-- Sean Silva</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 23, 2015 at 1:54 PM, Nathan Slingerland via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: slingn<br>
Date: Mon Nov 23 15:54:22 2015<br>
New Revision: 253921<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253921&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253921&view=rev</a><br>
Log:<br>
[Support] Add optional argument to SaturatingAdd() and SaturatingMultiply() to indicate that overflow occurred<br>
<br>
Summary: Adds the ability for callers to detect when saturation occurred on the result of saturating addition/multiplication.<br>
<br>
Reviewers: davidxl, silvas, rsmith<br>
<br>
Subscribers: llvm-commits<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D14931" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14931</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/MathExtras.h<br>
llvm/trunk/unittests/Support/MathExtrasTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/MathExtras.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253921&r1=253920&r2=253921&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253921&r1=253920&r2=253921&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/MathExtras.h (original)<br>
+++ llvm/trunk/include/llvm/Support/MathExtras.h Mon Nov 23 15:54:22 2015<br>
@@ -655,46 +655,79 @@ inline int64_t SignExtend64(uint64_t X,<br>
<br>
/// \brief Add two unsigned integers, X and Y, of type T.<br>
/// Clamp the result to the maximum representable value of T on overflow.<br>
+/// ResultOverflowed indicates if the result is larger than the maximum<br>
+/// representable value of type T.<br>
template <typename T><br>
typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
-SaturatingAdd(T X, T Y) {<br>
+SaturatingAdd(T X, T Y, bool &ResultOverflowed) {<br>
// Hacker's Delight, p. 29<br>
T Z = X + Y;<br>
- if (Z < X || Z < Y)<br>
+ ResultOverflowed = (Z < X || Z < Y);<br>
+ if (ResultOverflowed)<br>
return std::numeric_limits<T>::max();<br>
else<br>
return Z;<br>
}<br>
<br>
+/// \brief Add two unsigned integers, X and Y, of type T.<br>
+/// Clamp the result to the maximum representable value of T on overflow.<br>
+template <typename T><br>
+typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
+SaturatingAdd(T X, T Y) {<br>
+ bool ResultOverflowed;<br>
+ return SaturatingAdd(X, Y, ResultOverflowed);<br>
+}<br>
+<br>
/// \brief Multiply two unsigned integers, X and Y, of type T.<br>
/// Clamp the result to the maximum representable value of T on overflow.<br>
+/// ResultOverflowed indicates if the result is larger than the maximum<br>
+/// representable value of type T.<br>
template <typename T><br>
typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
-SaturatingMultiply(T X, T Y) {<br>
+SaturatingMultiply(T X, T Y, bool &ResultOverflowed) {<br>
// Hacker's Delight, p. 30 has a different algorithm, but we don't use that<br>
// because it fails for uint16_t (where multiplication can have undefined<br>
// behavior due to promotion to int), and requires a division in addition<br>
// to the multiplication.<br>
<br>
+ ResultOverflowed = false;<br>
+<br>
// Log2(Z) would be either Log2Z or Log2Z + 1.<br>
// Special case: if X or Y is 0, Log2_64 gives -1, and Log2Z<br>
// will necessarily be less than Log2Max as desired.<br>
int Log2Z = Log2_64(X) + Log2_64(Y);<br>
const T Max = std::numeric_limits<T>::max();<br>
int Log2Max = Log2_64(Max);<br>
- if (Log2Z < Log2Max)<br>
+ if (Log2Z < Log2Max) {<br>
return X * Y;<br>
- if (Log2Z > Log2Max)<br>
+ }<br>
+ if (Log2Z > Log2Max) {<br>
+ ResultOverflowed = true;<br>
return Max;<br>
+ }<br>
<br>
// We're going to use the top bit, and maybe overflow one<br>
// bit past it. Multiply all but the bottom bit then add<br>
// that on at the end.<br>
T Z = (X >> 1) * Y;<br>
- if (Z & ~(Max >> 1))<br>
+ if (Z & ~(Max >> 1)) {<br>
+ ResultOverflowed = true;<br>
return Max;<br>
+ }<br>
Z <<= 1;<br>
- return (X & 1) ? SaturatingAdd(Z, Y) : Z;<br>
+ if (X & 1)<br>
+ return SaturatingAdd(Z, Y, ResultOverflowed);<br>
+<br>
+ return Z;<br>
+}<br>
+<br>
+/// \brief Multiply two unsigned integers, X and Y, of type T.<br>
+/// Clamp the result to the maximum representable value of T on overflow.<br>
+template <typename T><br>
+typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
+SaturatingMultiply(T X, T Y) {<br>
+ bool ResultOverflowed;<br>
+ return SaturatingMultiply(X, Y, ResultOverflowed);<br>
}<br>
<br>
extern const float huge_valf;<br>
<br>
Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253921&r1=253920&r2=253921&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253921&r1=253920&r2=253921&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Mon Nov 23 15:54:22 2015<br>
@@ -194,10 +194,27 @@ template<typename T><br>
void SaturatingAddTestHelper()<br>
{<br>
const T Max = std::numeric_limits<T>::max();<br>
+ bool ResultOverflowed;<br>
+<br>
EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2)));<br>
+ EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(Max, SaturatingAdd(Max, T(1)));<br>
+ EXPECT_EQ(Max, SaturatingAdd(Max, T(1), ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
+<br>
+ EXPECT_EQ(Max, SaturatingAdd(T(1), T(Max - 1)));<br>
+ EXPECT_EQ(Max, SaturatingAdd(T(1), T(Max - 1), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(Max, SaturatingAdd(T(1), Max));<br>
+ EXPECT_EQ(Max, SaturatingAdd(T(1), Max, ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(Max, SaturatingAdd(Max, Max));<br>
+ EXPECT_EQ(Max, SaturatingAdd(Max, Max, ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
}<br>
<br>
TEST(MathExtras, SaturatingAdd) {<br>
@@ -211,22 +228,50 @@ template<typename T><br>
void SaturatingMultiplyTestHelper()<br>
{<br>
const T Max = std::numeric_limits<T>::max();<br>
+ bool ResultOverflowed;<br>
<br>
// Test basic multiplication.<br>
EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3)));<br>
+ EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(T(6), SaturatingMultiply(T(3), T(2)));<br>
+ EXPECT_EQ(T(6), SaturatingMultiply(T(3), T(2), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
<br>
// Test multiplication by zero.<br>
EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(0)));<br>
+ EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(0), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0)));<br>
+ EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1)));<br>
+ EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(T(0), SaturatingMultiply(Max, T(0)));<br>
+ EXPECT_EQ(T(0), SaturatingMultiply(Max, T(0), ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(T(0), SaturatingMultiply(T(0), Max));<br>
+ EXPECT_EQ(T(0), SaturatingMultiply(T(0), Max, ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
<br>
// Test multiplication by maximum value.<br>
EXPECT_EQ(Max, SaturatingMultiply(Max, T(2)));<br>
- EXPECT_EQ(Max, SaturatingMultiply(T(2),Max));<br>
+ EXPECT_EQ(Max, SaturatingMultiply(Max, T(2), ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
+<br>
+ EXPECT_EQ(Max, SaturatingMultiply(T(2), Max));<br>
+ EXPECT_EQ(Max, SaturatingMultiply(T(2), Max, ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
+<br>
EXPECT_EQ(Max, SaturatingMultiply(Max, Max));<br>
+ EXPECT_EQ(Max, SaturatingMultiply(Max, Max, ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
<br>
// Test interesting boundary conditions for algorithm -<br>
// ((1 << A) - 1) * ((1 << B) + K) for K in [-1, 0, 1]<br>
@@ -241,8 +286,12 @@ void SaturatingMultiplyTestHelper()<br>
<br>
if(OverflowExpected) {<br>
EXPECT_EQ(Max, SaturatingMultiply(X, Y));<br>
+ EXPECT_EQ(Max, SaturatingMultiply(X, Y, ResultOverflowed));<br>
+ EXPECT_TRUE(ResultOverflowed);<br>
} else {<br>
EXPECT_EQ(X * Y, SaturatingMultiply(X, Y));<br>
+ EXPECT_EQ(X * Y, SaturatingMultiply(X, Y, ResultOverflowed));<br>
+ EXPECT_FALSE(ResultOverflowed);<br>
}<br>
}<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>