[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Nathan Slingerland via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 20 09:33:21 PST 2015
Hi Richard,
Thank you for this patch.
It looks good to me, but just to be sure I measured the performance of the
two algorithms on x86_64 with clang 3.7 on a Haswell-E (5960X):
SaturatingMultiply2
stand-alone codegen: http://pastebin.com/ZysuRYZM
SaturatingMultiply3
stand-alone codegen: http://pastebin.com/7LSLaLcz
Test harness: http://pastebin.com/F7J2ZGuz
$ clang++-3.7 -O3 -std=c++11 test.cpp -o test
$ ./test `echo 2^32 | bc` `echo 2^33 | bc`
SaturatingMultiply2(): 32011704 us
SaturatingMultiply3(): 6138941 us <--- Richard's patch
It sure looks worth doing.
I'll update D14845 with your SaturatingMultiply implementation.
-Nathan
On Thu, Nov 19, 2015 at 6:19 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> The code is indeed broken, as ubsan reports (see my response to that bug).
>
> What do you think of the attached alternative implementation? It should
> fix the signed integer overflow bug and is probably faster in all cases too
> (since it doesn't use a division to check for overflow).
>
> On Thu, Nov 19, 2015 at 3:53 PM, Nathan Slingerland via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> I've filed the issue as https://llvm.org/bugs/show_bug.cgi?id=25580
>>
>> I'd still suggest we move forward with the MathExtras.h patch since that
>> unblocks the unit tests and is no more expensive than the previous
>> implementation.
>>
>> On Thu, Nov 19, 2015 at 3:47 PM, Xinliang David Li <xinliangli at gmail.com>
>> wrote:
>>
>>> It might be better to extract a small reproducible and report to
>>> sanitizer folks. If it is a bug there -- we should fix it there.
>>>
>>> David
>>>
>>> On Thu, Nov 19, 2015 at 3:01 PM, Nathan Slingerland <slingn at gmail.com>
>>> wrote:
>>>
>>>> I am able to reproduce the failure using -fsanitizer=undefined (I tried
>>>> on Ubuntu 15.10 x86_64, clang 3.6).
>>>>
>>>> Test case: http://pastebin.com/Eejy5rNB
>>>>
>>>> $ clang++ -O1 -std=c++11 -fsanitize=undefined test.cpp -o test
>>>> $ ./test
>>>> main.cpp:25:11: runtime error: signed integer overflow: 65535 * 65535
>>>> cannot be represented in type 'int'
>>>>
>>>> Interestingly, the failure only occurs with uint16_t.
>>>> The other unsigned types (uint8_t/uint32_t/uint64_t) pass if I comment
>>>> out the uint16_t case.
>>>> It's not clear to me why this is triggering the signed multiply check
>>>> with uint16_t.
>>>>
>>>> I have put a fix (work-around, really) up for review as D14845.
>>>>
>>>>
>>>> On Wed, Nov 18, 2015 at 9:48 PM, Xinliang David Li via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> probably
>>>>>
>>>>> cmake -DLLVM_USE_SANITIZER=Undefined ...
>>>>>
>>>>> David
>>>>>
>>>>> On Wed, Nov 18, 2015 at 9:37 PM, Sean Silva via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> What is the configure line for this bot?
>>>>>>
>>>>>> On Wed, Nov 18, 2015 at 4:28 PM, Bruno Cardoso Lopes via llvm-commits
>>>>>> <llvm-commits at lists.llvm.org> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> 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/20151120/a2ee391a/attachment.html>
More information about the llvm-commits
mailing list