[llvm] r269131 - Reapply r266477 and r266488
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Tue May 10 17:19:47 PDT 2016
On Tue, May 10, 2016 at 4:41 PM, Easwaran Raman <eraman at google.com> wrote:
> (+cc: vsk)
> Hi Sean,
> I have reverted the patch. First, could you clarify the "core libraries"
> part? Is it libCore alone or a larger set?
>
Sorry, I used that term for the lack of a better description. More or less
I mean anything in lib/ of any LLVM subproject.
> I presume it is a larger set since this is part of libProfileData, but
> still wanted to check with you. I can think of a couple of ways to avoid
> the global: a) do not cache this at the ProfileSummary level and instead
> cache it at the client, b) move ProfileSummary to Support (or IR). What do
> you suggest is the best way forward?
>
I have not done an in-depth analysis of the situation, but I think this can
be solved by taking the "compute from metadata" design to its logical
conclusion and splitting the solution into two parts:
- there is a part that lives in the IR library that is computed purely
based on the metadata and has no dependence on ProfileData
- there is a part that lives in the the ProfileData library that does
InstrProf/SampleProf specific annotation of the metadata.
The annotating part needs the IR library, so it makes sense for some of the
shared routines to be in the IR library (such as the list of cutoffs).
-- Sean Silva
>
> Thanks,
> Easwaran
>
>
> On Tue, May 10, 2016 at 4:17 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Tue, May 10, 2016 at 3:03 PM, Easwaran Raman via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: eraman
>>> Date: Tue May 10 17:03:23 2016
>>> New Revision: 269131
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=269131&view=rev
>>> Log:
>>> Reapply r266477 and r266488
>>>
>>> Added:
>>> llvm/trunk/unittests/ProfileData/ProfileSummaryTest.cpp
>>> - copied unchanged from r266618,
>>> llvm/trunk/unittests/ProfileData/ProfileSummaryTest.cpp
>>> Modified:
>>> llvm/trunk/include/llvm/ProfileData/ProfileCommon.h
>>> llvm/trunk/lib/Analysis/InlineCost.cpp
>>> llvm/trunk/lib/Analysis/LLVMBuild.txt
>>> llvm/trunk/lib/ProfileData/ProfileSummary.cpp
>>> llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll
>>> llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll
>>> llvm/trunk/unittests/ProfileData/CMakeLists.txt
>>>
>>> Modified: llvm/trunk/include/llvm/ProfileData/ProfileCommon.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/ProfileCommon.h?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ProfileData/ProfileCommon.h (original)
>>> +++ llvm/trunk/include/llvm/ProfileData/ProfileCommon.h Tue May 10
>>> 17:03:23 2016
>>> @@ -21,6 +21,8 @@
>>> #include <vector>
>>>
>>> #include "llvm/Support/Casting.h"
>>> +#include "llvm/Support/ManagedStatic.h"
>>> +#include "llvm/Support/Mutex.h"
>>>
>>> namespace llvm {
>>> class Function;
>>> @@ -35,6 +37,7 @@ class LLVMContext;
>>> class Metadata;
>>> class MDTuple;
>>> class MDNode;
>>> +class Module;
>>>
>>> inline const char *getHotSectionPrefix() { return ".hot"; }
>>> inline const char *getUnlikelySectionPrefix() { return ".unlikely"; }
>>> @@ -67,6 +70,14 @@ private:
>>> // appears in the profile. The map is kept sorted in the descending
>>> order of
>>> // counts.
>>> std::map<uint64_t, uint32_t, std::greater<uint64_t>> CountFrequencies;
>>> + // Compute profile summary for a module.
>>> + static ProfileSummary *computeProfileSummary(Module *M);
>>> + // Cache of last seen module and its profile summary.
>>> + static ManagedStatic<std::pair<Module *,
>>> std::unique_ptr<ProfileSummary>>>
>>> + CachedSummary;
>>> + // Mutex to access summary cache
>>> + static ManagedStatic<sys::SmartMutex<true>> CacheMutex;
>>>
>>
>> Hi Easwaran,
>>
>> In the LLVM core libraries, we don't use globals like this.
>>
>> Can you please revert and redesign this to avoid the global? For example,
>> keeping it in the Module itself? I understand that there are some layering
>> issues here, and I'm glad to help with finding a nice solution, but putting
>> a global in the LLVM core libraries like this is not acceptable.
>>
>> Thanks,
>>
>> -- Sean Silva
>>
>>
>>> +
>>> protected:
>>> SummaryEntryVector DetailedSummary;
>>> std::vector<uint32_t> DetailedSummaryCutoffs;
>>> @@ -85,14 +96,12 @@ protected:
>>> : PSK(K), DetailedSummary(DetailedSummary),
>>> TotalCount(TotalCount),
>>> MaxCount(MaxCount), MaxFunctionCount(MaxFunctionCount),
>>> NumCounts(NumCounts), NumFunctions(NumFunctions) {}
>>> - ~ProfileSummary() = default;
>>> inline void addCount(uint64_t Count);
>>> /// \brief Return metadata specific to the profile format.
>>> /// Derived classes implement this method to return a vector of
>>> Metadata.
>>> virtual std::vector<Metadata *> getFormatSpecificMD(LLVMContext
>>> &Context) = 0;
>>> /// \brief Return detailed summary as metadata.
>>> Metadata *getDetailedSummaryMD(LLVMContext &Context);
>>> -
>>> public:
>>> static const int Scale = 1000000;
>>> Kind getKind() const { return PSK; }
>>> @@ -111,6 +120,10 @@ public:
>>> static ProfileSummary *getFromMD(Metadata *MD);
>>> uint32_t getNumFunctions() { return NumFunctions; }
>>> uint64_t getMaxFunctionCount() { return MaxFunctionCount; }
>>> + /// \brief Get profile summary associated with module \p M
>>> + static inline ProfileSummary *getProfileSummary(Module *M);
>>> + virtual ~ProfileSummary() = default;
>>> + virtual bool operator==(ProfileSummary &Other);
>>> };
>>>
>>> class InstrProfSummary final : public ProfileSummary {
>>> @@ -140,6 +153,7 @@ public:
>>> uint64_t getTotalCount() { return TotalCount; }
>>> uint64_t getMaxBlockCount() { return MaxCount; }
>>> uint64_t getMaxInternalBlockCount() { return MaxInternalBlockCount; }
>>> + bool operator==(ProfileSummary &Other) override;
>>> };
>>>
>>> class SampleProfileSummary final : public ProfileSummary {
>>> @@ -180,5 +194,24 @@ SummaryEntryVector &ProfileSummary::getD
>>> return DetailedSummary;
>>> }
>>>
>>> +ProfileSummary *ProfileSummary::getProfileSummary(Module *M) {
>>> + if (!M)
>>> + return nullptr;
>>> + sys::SmartScopedLock<true> Lock(*CacheMutex);
>>> + // Computing profile summary for a module involves parsing a fairly
>>> large
>>> + // metadata and could be expensive. We use a simple cache of the last
>>> seen
>>> + // module and its profile summary.
>>> + if (CachedSummary->first != M) {
>>> + auto *Summary = computeProfileSummary(M);
>>> + // Do not cache if the summary is empty. This is because a later
>>> pass
>>> + // (sample profile loader, for example) could attach the summary
>>> metadata on
>>> + // the module.
>>> + if (!Summary)
>>> + return nullptr;
>>> + CachedSummary->first = M;
>>> + CachedSummary->second.reset(Summary);
>>> + }
>>> + return CachedSummary->second.get();
>>> +}
>>> } // end namespace llvm
>>> #endif
>>>
>>> Modified: llvm/trunk/lib/Analysis/InlineCost.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Analysis/InlineCost.cpp (original)
>>> +++ llvm/trunk/lib/Analysis/InlineCost.cpp Tue May 10 17:03:23 2016
>>> @@ -30,6 +30,7 @@
>>> #include "llvm/IR/InstVisitor.h"
>>> #include "llvm/IR/IntrinsicInst.h"
>>> #include "llvm/IR/Operator.h"
>>> +#include "llvm/ProfileData/ProfileCommon.h"
>>> #include "llvm/Support/Debug.h"
>>> #include "llvm/Support/raw_ostream.h"
>>>
>>> @@ -630,10 +631,11 @@ void CallAnalyzer::updateThreshold(CallS
>>> // a well-tuned heuristic based on *callsite* hotness and not callee
>>> hotness.
>>> uint64_t FunctionCount = 0, MaxFunctionCount = 0;
>>> bool HasPGOCounts = false;
>>> - if (Callee.getEntryCount() &&
>>> Callee.getParent()->getMaximumFunctionCount()) {
>>> + ProfileSummary *PS =
>>> ProfileSummary::getProfileSummary(Callee.getParent());
>>> + if (Callee.getEntryCount() && PS) {
>>> HasPGOCounts = true;
>>> FunctionCount = Callee.getEntryCount().getValue();
>>> - MaxFunctionCount =
>>> Callee.getParent()->getMaximumFunctionCount().getValue();
>>> + MaxFunctionCount = PS->getMaxFunctionCount();
>>> }
>>>
>>> // Listen to the inlinehint attribute or profile based hotness
>>> information
>>>
>>> Modified: llvm/trunk/lib/Analysis/LLVMBuild.txt
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LLVMBuild.txt?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Analysis/LLVMBuild.txt (original)
>>> +++ llvm/trunk/lib/Analysis/LLVMBuild.txt Tue May 10 17:03:23 2016
>>> @@ -19,4 +19,4 @@
>>> type = Library
>>> name = Analysis
>>> parent = Libraries
>>> -required_libraries = Core Support
>>> +required_libraries = Core ProfileData Support
>>>
>>> Modified: llvm/trunk/lib/ProfileData/ProfileSummary.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/ProfileSummary.cpp?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/ProfileData/ProfileSummary.cpp (original)
>>> +++ llvm/trunk/lib/ProfileData/ProfileSummary.cpp Tue May 10 17:03:23
>>> 2016
>>> @@ -15,6 +15,7 @@
>>> #include "llvm/IR/Constants.h"
>>> #include "llvm/IR/Function.h"
>>> #include "llvm/IR/Metadata.h"
>>> +#include "llvm/IR/Module.h"
>>> #include "llvm/IR/Type.h"
>>> #include "llvm/ProfileData/InstrProf.h"
>>> #include "llvm/ProfileData/ProfileCommon.h"
>>> @@ -32,6 +33,10 @@ const std::vector<uint32_t> ProfileSumma
>>> 900000, 950000, 990000, 999000, 999900, 999990, 999999});
>>> const char *ProfileSummary::KindStr[2] = {"InstrProf", "SampleProfile"};
>>>
>>> +ManagedStatic<std::pair<Module *, std::unique_ptr<ProfileSummary>>>
>>> + ProfileSummary::CachedSummary;
>>> +ManagedStatic<sys::SmartMutex<true>> ProfileSummary::CacheMutex;
>>> +
>>> void InstrProfSummary::addRecord(const InstrProfRecord &R) {
>>> // The first counter is not necessarily an entry count for IR
>>> // instrumentation profiles.
>>> @@ -86,6 +91,39 @@ void ProfileSummary::computeDetailedSumm
>>> }
>>> }
>>>
>>> +bool ProfileSummary::operator==(ProfileSummary &Other) {
>>> + if (getKind() != Other.getKind())
>>> + return false;
>>> + if (TotalCount != Other.TotalCount)
>>> + return false;
>>> + if (MaxCount != Other.MaxCount)
>>> + return false;
>>> + if (MaxFunctionCount != Other.MaxFunctionCount)
>>> + return false;
>>> + if (NumFunctions != Other.NumFunctions)
>>> + return false;
>>> + if (NumCounts != Other.NumCounts)
>>> + return false;
>>> + std::vector<ProfileSummaryEntry> DS1 = getDetailedSummary();
>>> + std::vector<ProfileSummaryEntry> DS2 = Other.getDetailedSummary();
>>> + auto CompareSummaryEntry = [](ProfileSummaryEntry &E1,
>>> + ProfileSummaryEntry &E2) {
>>> + return E1.Cutoff == E2.Cutoff && E1.MinCount == E2.MinCount &&
>>> + E1.NumCounts == E2.NumCounts;
>>> + };
>>> + if (!std::equal(DS1.begin(), DS1.end(), DS2.begin(),
>>> CompareSummaryEntry))
>>> + return false;
>>> + return true;
>>> +}
>>> +
>>> +bool InstrProfSummary::operator==(ProfileSummary &Other) {
>>> + InstrProfSummary *OtherIPS = dyn_cast<InstrProfSummary>(&Other);
>>> + if (!OtherIPS)
>>> + return false;
>>> + return MaxInternalBlockCount == OtherIPS->MaxInternalBlockCount &&
>>> + ProfileSummary::operator==(Other);
>>> +}
>>> +
>>> // Returns true if the function is a hot function.
>>> bool ProfileSummary::isFunctionHot(const Function *F) {
>>> // FIXME: update when summary data is stored in module's metadata.
>>> @@ -366,3 +404,9 @@ ProfileSummary *ProfileSummary::getFromM
>>> else
>>> return nullptr;
>>> }
>>> +
>>> +ProfileSummary *ProfileSummary::computeProfileSummary(Module *M) {
>>> + if (Metadata *MD = M->getProfileSummary())
>>> + return getFromMD(MD);
>>> + return nullptr;
>>> +}
>>>
>>> Modified: llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll (original)
>>> +++ llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll Tue May 10
>>> 17:03:23 2016
>>> @@ -5,7 +5,7 @@
>>> ; A callee with identical body does gets inlined because cost fits
>>> within the
>>> ; inline-threshold
>>>
>>> -define i32 @callee1(i32 %x) !prof !1 {
>>> +define i32 @callee1(i32 %x) !prof !21 {
>>> %x1 = add i32 %x, 1
>>> %x2 = add i32 %x1, 1
>>> %x3 = add i32 %x2, 1
>>> @@ -13,7 +13,7 @@ define i32 @callee1(i32 %x) !prof !1 {
>>> ret i32 %x3
>>> }
>>>
>>> -define i32 @callee2(i32 %x) !prof !2 {
>>> +define i32 @callee2(i32 %x) !prof !22 {
>>> ; CHECK-LABEL: @callee2(
>>> %x1 = add i32 %x, 1
>>> %x2 = add i32 %x1, 1
>>> @@ -22,7 +22,7 @@ define i32 @callee2(i32 %x) !prof !2 {
>>> ret i32 %x3
>>> }
>>>
>>> -define i32 @caller2(i32 %y1) !prof !2 {
>>> +define i32 @caller2(i32 %y1) !prof !22 {
>>> ; CHECK-LABEL: @caller2(
>>> ; CHECK: call i32 @callee2
>>> ; CHECK-NOT: call i32 @callee1
>>> @@ -32,8 +32,19 @@ define i32 @caller2(i32 %y1) !prof !2 {
>>> ret i32 %y3
>>> }
>>>
>>> -!llvm.module.flags = !{!0}
>>> -!0 = !{i32 1, !"MaxFunctionCount", i32 1000}
>>> -!1 = !{!"function_entry_count", i64 100}
>>> -!2 = !{!"function_entry_count", i64 1}
>>> -
>>> +!llvm.module.flags = !{!1}
>>> +!21 = !{!"function_entry_count", i64 100}
>>> +!22 = !{!"function_entry_count", i64 1}
>>> +
>>> +!1 = !{i32 1, !"ProfileSummary", !2}
>>> +!2 = !{!3, !4, !5, !6, !7, !8, !9, !10}
>>> +!3 = !{!"ProfileFormat", !"InstrProf"}
>>> +!4 = !{!"TotalCount", i64 10000}
>>> +!5 = !{!"MaxBlockCount", i64 1000}
>>> +!6 = !{!"MaxInternalBlockCount", i64 1}
>>> +!7 = !{!"MaxFunctionCount", i64 1000}
>>> +!8 = !{!"NumBlocks", i64 3}
>>> +!9 = !{!"NumFunctions", i64 3}
>>> +!10 = !{!"DetailedSummary", !11}
>>> +!11 = !{!12}
>>> +!12 = !{i32 10000, i64 0, i32 0}
>>>
>>> Modified: llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll (original)
>>> +++ llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll Tue May 10
>>> 17:03:23 2016
>>> @@ -5,7 +5,7 @@
>>> ; A cold callee with identical body does not get inlined because cost
>>> exceeds the
>>> ; inline-threshold
>>>
>>> -define i32 @callee1(i32 %x) !prof !1 {
>>> +define i32 @callee1(i32 %x) !prof !20 {
>>> %x1 = add i32 %x, 1
>>> %x2 = add i32 %x1, 1
>>> %x3 = add i32 %x2, 1
>>> @@ -13,7 +13,7 @@ define i32 @callee1(i32 %x) !prof !1 {
>>> ret i32 %x3
>>> }
>>>
>>> -define i32 @callee2(i32 %x) !prof !2 {
>>> +define i32 @callee2(i32 %x) !prof !21 {
>>> ; CHECK-LABEL: @callee2(
>>> %x1 = add i32 %x, 1
>>> %x2 = add i32 %x1, 1
>>> @@ -22,7 +22,7 @@ define i32 @callee2(i32 %x) !prof !2 {
>>> ret i32 %x3
>>> }
>>>
>>> -define i32 @caller2(i32 %y1) !prof !2 {
>>> +define i32 @caller2(i32 %y1) !prof !21 {
>>> ; CHECK-LABEL: @caller2(
>>> ; CHECK: call i32 @callee2
>>> ; CHECK-NOT: call i32 @callee1
>>> @@ -32,8 +32,19 @@ define i32 @caller2(i32 %y1) !prof !2 {
>>> ret i32 %y3
>>> }
>>>
>>> -!llvm.module.flags = !{!0}
>>> -!0 = !{i32 1, !"MaxFunctionCount", i32 10}
>>> -!1 = !{!"function_entry_count", i64 10}
>>> -!2 = !{!"function_entry_count", i64 1}
>>> -
>>> +!llvm.module.flags = !{!1}
>>> +!20 = !{!"function_entry_count", i64 10}
>>> +!21 = !{!"function_entry_count", i64 1}
>>> +
>>> +!1 = !{i32 1, !"ProfileSummary", !2}
>>> +!2 = !{!3, !4, !5, !6, !7, !8, !9, !10}
>>> +!3 = !{!"ProfileFormat", !"InstrProf"}
>>> +!4 = !{!"TotalCount", i64 10000}
>>> +!5 = !{!"MaxBlockCount", i64 10}
>>> +!6 = !{!"MaxInternalBlockCount", i64 1}
>>> +!7 = !{!"MaxFunctionCount", i64 10}
>>> +!8 = !{!"NumBlocks", i64 3}
>>> +!9 = !{!"NumFunctions", i64 3}
>>> +!10 = !{!"DetailedSummary", !11}
>>> +!11 = !{!12}
>>> +!12 = !{i32 10000, i64 0, i32 0}
>>>
>>> Modified: llvm/trunk/unittests/ProfileData/CMakeLists.txt
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/CMakeLists.txt?rev=269131&r1=269130&r2=269131&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/unittests/ProfileData/CMakeLists.txt (original)
>>> +++ llvm/trunk/unittests/ProfileData/CMakeLists.txt Tue May 10 17:03:23
>>> 2016
>>> @@ -8,5 +8,6 @@ set(LLVM_LINK_COMPONENTS
>>> add_llvm_unittest(ProfileDataTests
>>> CoverageMappingTest.cpp
>>> InstrProfTest.cpp
>>> + ProfileSummaryTest.cpp
>>> SampleProfTest.cpp
>>> )
>>>
>>>
>>> _______________________________________________
>>> 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/20160510/acb5601f/attachment.html>
More information about the llvm-commits
mailing list