[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Nathan Slingerland via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 20:10:29 PST 2015
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151118/3093a5cc/attachment-0001.html>
More information about the llvm-commits
mailing list