[llvm] r253412 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions
Nathan Slingerland via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 12:42:41 PST 2015
Thanks Sean.
I have updated the test case based on your comments.
On Tue, Nov 17, 2015 at 8:04 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Tue, Nov 17, 2015 at 4:52 PM, Nathan Slingerland via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: slingn
>> Date: Tue Nov 17 18:52:43 2015
>> New Revision: 253412
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=253412&view=rev
>> Log:
>> [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions
>>
>> Summary:
>> This change adds MathExtras helper functions for handling unsigned,
>> saturating addition and multiplication. It also updates the instrumentation
>> and sample profile merge implementations to use them.
>>
>> No functional changes.
>>
>> Reviewers: dnovillo, bogner, davidxl
>>
>> Subscribers: davidxl, llvm-commits
>>
>> Differential Revision: http://reviews.llvm.org/D14720
>>
>> Modified:
>> llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> llvm/trunk/include/llvm/ProfileData/SampleProf.h
>> llvm/trunk/include/llvm/Support/MathExtras.h
>> llvm/trunk/unittests/Support/MathExtrasTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=253412&r1=253411&r2=253412&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Tue Nov 17 18:52:43
>> 2015
>> @@ -226,7 +226,7 @@ struct InstrProfValueSiteRecord {
>> while (I != IE && I->Value < J->Value)
>> ++I;
>> if (I != IE && I->Value == J->Value) {
>> - I->Count += J->Count;
>> + I->Count = SaturatingAdd(I->Count, J->Count);
>> ++I;
>> continue;
>> }
>>
>> Modified: llvm/trunk/include/llvm/ProfileData/SampleProf.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/SampleProf.h?rev=253412&r1=253411&r2=253412&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ProfileData/SampleProf.h (original)
>> +++ llvm/trunk/include/llvm/ProfileData/SampleProf.h Tue Nov 17 18:52:43
>> 2015
>> @@ -173,10 +173,7 @@ public:
>> /// Sample counts accumulate using saturating arithmetic, to avoid
>> wrapping
>> /// around unsigned integers.
>> void addSamples(uint64_t S) {
>> - if (NumSamples <= std::numeric_limits<uint64_t>::max() - S)
>> - NumSamples += S;
>> - else
>> - NumSamples = std::numeric_limits<uint64_t>::max();
>> + NumSamples = SaturatingAdd(NumSamples, S);
>> }
>>
>> /// Add called function \p F with samples \p S.
>> @@ -185,10 +182,7 @@ public:
>> /// around unsigned integers.
>> void addCalledTarget(StringRef F, uint64_t S) {
>> uint64_t &TargetSamples = CallTargets[F];
>> - if (TargetSamples <= std::numeric_limits<uint64_t>::max() - S)
>> - TargetSamples += S;
>> - else
>> - TargetSamples = std::numeric_limits<uint64_t>::max();
>> + TargetSamples = SaturatingAdd(TargetSamples, S);
>> }
>>
>> /// Return true if this sample record contains function calls.
>>
>> Modified: llvm/trunk/include/llvm/Support/MathExtras.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253412&r1=253411&r2=253412&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/MathExtras.h (original)
>> +++ llvm/trunk/include/llvm/Support/MathExtras.h Tue Nov 17 18:52:43 2015
>> @@ -653,6 +653,32 @@ inline int64_t SignExtend64(uint64_t X,
>> return int64_t(X << (64 - B)) >> (64 - B);
>> }
>>
>> +/// \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) {
>> + // Hacker's Delight, p. 29
>> + T Z = X + Y;
>> + if (Z < X || Z < Y)
>> + return std::numeric_limits<T>::max();
>> + else
>> + 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) {
>> + // Hacker's Delight, p. 30
>> + T Z = X * Y;
>> + if (Y != 0 && Z / Y != X)
>> + return std::numeric_limits<T>::max();
>> + else
>> + return Z;
>> +}
>> +
>> extern const float huge_valf;
>> } // End llvm namespace
>>
>>
>> Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253412&r1=253411&r2=253412&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)
>> +++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Tue Nov 17 18:52:43
>> 2015
>> @@ -190,4 +190,52 @@ TEST(MathExtras, RoundUpToAlignment) {
>> EXPECT_EQ(552u, RoundUpToAlignment(321, 255, 42));
>> }
>>
>> +template<typename T>
>> +void SaturatingAddTestHelper()
>> +{
>> + EXPECT_EQ(static_cast<T>(3),
>> + SaturatingAdd(static_cast<T>(1), static_cast<T>(2)));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingAdd(std::numeric_limits<T>::max(),
>> static_cast<T>(1)));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingAdd(static_cast<T>(1),
>> std::numeric_limits<T>::max()));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingAdd(std::numeric_limits<T>::max(),
>> + std::numeric_limits<T>::max()));
>> +}
>> +
>> +TEST(MathExtras, SaturatingAdd) {
>> + SaturatingAddTestHelper<uint8_t>();
>> + SaturatingAddTestHelper<uint16_t>();
>> + SaturatingAddTestHelper<uint32_t>();
>> + SaturatingAddTestHelper<uint64_t>();
>> +}
>> +
>> +template<typename T>
>> +void SaturatingMultiplyTestHelper()
>> +{
>> + EXPECT_EQ(static_cast<T>(0),
>> + SaturatingMultiply(static_cast<T>(1), static_cast<T>(0)));
>> + EXPECT_EQ(static_cast<T>(0),
>> + SaturatingMultiply(static_cast<T>(0), static_cast<T>(1)));
>> + EXPECT_EQ(static_cast<T>(6),
>> + SaturatingMultiply(static_cast<T>(2), static_cast<T>(3)));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingMultiply(std::numeric_limits<T>::max(),
>> + static_cast<T>(2)));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingMultiply(static_cast<T>(2),
>> + std::numeric_limits<T>::max()));
>> + EXPECT_EQ(std::numeric_limits<T>::max(),
>> + SaturatingMultiply(std::numeric_limits<T>::max(),
>> + std::numeric_limits<T>::max()));
>>
>
> I think adding a helper:
>
> T Max = std::numeric_limits<T>::max();
>
> would help clean these up. Also, I think instead of `static_cast<T>(1)`
> you can do just `T(1)`.
>
> So for example that second-to-last one becomes:
>
> EXPECT_EQ(Max, SaturatingMultiply(T(2), Max))
>
> which seems like a nice improvement.
>
> -- Sean Silva
>
>
>> +}
>> +
>> +TEST(MathExtras, SaturatingMultiply) {
>> + SaturatingMultiplyTestHelper<uint8_t>();
>> + SaturatingMultiplyTestHelper<uint16_t>();
>> + SaturatingMultiplyTestHelper<uint32_t>();
>> + SaturatingMultiplyTestHelper<uint64_t>();
>> +}
>> +
>> }
>>
>>
>> _______________________________________________
>> 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/20151118/c508412a/attachment.html>
More information about the llvm-commits
mailing list