[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 20:41:56 PST 2015
The new overflow test is testing unsigned integer type. Why is ubsan
complaining about int type?
Nathan, you can probably remove the test first while investigating.
David
On Wed, Nov 18, 2015 at 8:10 PM, Nathan Slingerland via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Thanks Bruno, I am looking into it.
>
> On Wed, Nov 18, 2015 at 4:28 PM, Bruno Cardoso Lopes <
> bruno.cardoso at gmail.com> wrote:
>
>> Hi Nathan,
>>
>> Ubsan is detecting some undefined behavior after your commit:
>> http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/746/
>>
>> ******************** TEST 'LLVM-Unit ::
>> Support/SupportTests/MathExtras.SaturatingMultiply' FAILED
>> ********************
>> 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
>> /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan at 2
>> /llvm/include/llvm/Support/MathExtras.h:675:11:
>> runtime error: signed integer overflow: 65535 * 65535 cannot be
>> represented in type 'int'
>> SUMMARY: AddressSanitizer: undefined-behavior
>> /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan at 2
>> /llvm/include/llvm/Support/MathExtras.h:675:11
>> in
>> 0 SupportTests 0x0000000105b92545
>> llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 277
>> 1 SupportTests 0x0000000105b90400
>> llvm::sys::RunSignalHandlers() + 448
>> 2 SupportTests 0x0000000105b937d3
>> SignalHandler(int) + 611
>> 3 libsystem_platform.dylib 0x00007fff8d239f1a _sigtramp + 26
>> 4 libsystem_platform.dylib 0x0000000000000003 _sigtramp +
>> 1927045379
>> 5 libsystem_c.dylib 0x00007fff88edd9b3 abort + 129
>> 6 libclang_rt.asan_osx_dynamic.dylib 0x00000001060c0f76
>> __sanitizer::Abort() + 6
>>
>>
>>
>> On Wed, Nov 18, 2015 at 12:40 PM, Nathan Slingerland via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> > Author: slingn
>> > Date: Wed Nov 18 14:40:41 2015
>> > New Revision: 253497
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=253497&view=rev
>> > Log:
>> > [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions
>> (2nd try)
>> >
>> > 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.
>> >
>> > 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/ProfileData/InstrProfTest.cpp
>> > 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=253497&r1=253496&r2=253497&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> > +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Nov 18 14:40:41
>> 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=253497&r1=253496&r2=253497&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/ProfileData/SampleProf.h (original)
>> > +++ llvm/trunk/include/llvm/ProfileData/SampleProf.h Wed Nov 18
>> 14:40:41 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=253497&r1=253496&r2=253497&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/Support/MathExtras.h (original)
>> > +++ llvm/trunk/include/llvm/Support/MathExtras.h Wed Nov 18 14:40:41
>> 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/ProfileData/InstrProfTest.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=253497&r1=253496&r2=253497&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
>> > +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Nov 18
>> 14:40:41 2015
>> > @@ -350,6 +350,38 @@ TEST_F(InstrProfTest, get_icall_data_mer
>> > ASSERT_EQ(2U, VD_4[2].Count);
>> > }
>> >
>> > +TEST_F(InstrProfTest, get_icall_data_merge1_saturation) {
>> > + const uint64_t Max = std::numeric_limits<uint64_t>::max();
>> > +
>> > + InstrProfRecord Record1("caller", 0x1234, {1});
>> > + InstrProfRecord Record2("caller", 0x1234, {1});
>> > + InstrProfRecord Record3("callee1", 0x1235, {3, 4});
>> > +
>> > + Record1.reserveSites(IPVK_IndirectCallTarget, 1);
>> > + InstrProfValueData VD1[] = {{(uint64_t) "callee1", 1}};
>> > + Record1.addValueData(IPVK_IndirectCallTarget, 0, VD1, 1, nullptr);
>> > +
>> > + Record2.reserveSites(IPVK_IndirectCallTarget, 1);
>> > + InstrProfValueData VD2[] = {{(uint64_t) "callee1", Max}};
>> > + Record2.addValueData(IPVK_IndirectCallTarget, 0, VD2, 1, nullptr);
>> > +
>> > + Writer.addRecord(std::move(Record1));
>> > + Writer.addRecord(std::move(Record2));
>> > + Writer.addRecord(std::move(Record3));
>> > +
>> > + auto Profile = Writer.writeBuffer();
>> > + readProfile(std::move(Profile));
>> > +
>> > + // Verify saturation of counts.
>> > + ErrorOr<InstrProfRecord> R = Reader->getInstrProfRecord("caller",
>> 0x1234);
>> > + ASSERT_TRUE(NoError(R.getError()));
>> > + ASSERT_EQ(1U, R.get().getNumValueSites(IPVK_IndirectCallTarget));
>> > + std::unique_ptr<InstrProfValueData[]> VD =
>> > + R.get().getValueForSite(IPVK_IndirectCallTarget, 0);
>> > + ASSERT_EQ(StringRef("callee1"), StringRef((const char *)VD[0].Value,
>> 7));
>> > + ASSERT_EQ(Max, VD[0].Count);
>> > +}
>> > +
>> > TEST_F(InstrProfTest, get_max_function_count) {
>> > InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});
>> > InstrProfRecord Record2("bar", 0, {1ULL << 63});
>> >
>> > Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253497&r1=253496&r2=253497&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)
>> > +++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Wed Nov 18 14:40:41
>> 2015
>> > @@ -190,4 +190,40 @@ TEST(MathExtras, RoundUpToAlignment) {
>> > EXPECT_EQ(552u, RoundUpToAlignment(321, 255, 42));
>> > }
>> >
>> > +template<typename T>
>> > +void SaturatingAddTestHelper()
>> > +{
>> > + const T Max = std::numeric_limits<T>::max();
>> > + EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2)));
>> > + EXPECT_EQ(Max, SaturatingAdd(Max, T(1)));
>> > + EXPECT_EQ(Max, SaturatingAdd(T(1), Max));
>> > + EXPECT_EQ(Max, SaturatingAdd(Max, Max));
>> > +}
>> > +
>> > +TEST(MathExtras, SaturatingAdd) {
>> > + SaturatingAddTestHelper<uint8_t>();
>> > + SaturatingAddTestHelper<uint16_t>();
>> > + SaturatingAddTestHelper<uint32_t>();
>> > + SaturatingAddTestHelper<uint64_t>();
>> > +}
>> > +
>> > +template<typename T>
>> > +void SaturatingMultiplyTestHelper()
>> > +{
>> > + const T Max = std::numeric_limits<T>::max();
>> > + EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0)));
>> > + EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1)));
>> > + EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3)));
>> > + EXPECT_EQ(Max, SaturatingMultiply(Max, T(2)));
>> > + EXPECT_EQ(Max, SaturatingMultiply(T(2),Max));
>> > + EXPECT_EQ(Max, SaturatingMultiply(Max, Max));
>> > +}
>> > +
>> > +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
>>
>>
>>
>> --
>> Bruno Cardoso Lopes
>> http://www.brunocardoso.cc
>>
>
>
> _______________________________________________
> 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/785d0645/attachment.html>
More information about the llvm-commits
mailing list