[llvm] r269131 - Reapply r266477 and r266488
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Tue May 10 21:55:53 PDT 2016
On Tue, May 10, 2016 at 5:19 PM, Sean Silva via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
>
> 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.
>
I like the this suggestion of splitting.
>
> 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).
>
I think it is better to put part 1 into Support lib -- both
BranchProbability and BlockFrequency representation lives there today.
David
> -- 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
>>>>
>>>
>>>
>>
>
> _______________________________________________
> 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/95b5058d/attachment.html>
More information about the llvm-commits
mailing list