<div dir="ltr"><div>(+cc: vsk)</div>Hi Sean,<div> I have reverted the patch. First, could you clarify the "core libraries" part? Is it libCore alone or a larger set? 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?</div><div><br></div><div>Thanks,</div><div>Easwaran</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 10, 2016 at 4:17 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, May 10, 2016 at 3:03 PM, Easwaran Raman via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: eraman<br>
Date: Tue May 10 17:03:23 2016<br>
New Revision: 269131<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=269131&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=269131&view=rev</a><br>
Log:<br>
Reapply r266477 and r266488<br>
<br>
Added:<br>
llvm/trunk/unittests/ProfileData/ProfileSummaryTest.cpp<br>
- copied unchanged from r266618, llvm/trunk/unittests/ProfileData/ProfileSummaryTest.cpp<br>
Modified:<br>
llvm/trunk/include/llvm/ProfileData/ProfileCommon.h<br>
llvm/trunk/lib/Analysis/InlineCost.cpp<br>
llvm/trunk/lib/Analysis/LLVMBuild.txt<br>
llvm/trunk/lib/ProfileData/ProfileSummary.cpp<br>
llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll<br>
llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll<br>
llvm/trunk/unittests/ProfileData/CMakeLists.txt<br>
<br>
Modified: llvm/trunk/include/llvm/ProfileData/ProfileCommon.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/ProfileCommon.h?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/ProfileCommon.h?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ProfileData/ProfileCommon.h (original)<br>
+++ llvm/trunk/include/llvm/ProfileData/ProfileCommon.h Tue May 10 17:03:23 2016<br>
@@ -21,6 +21,8 @@<br>
#include <vector><br>
<br>
#include "llvm/Support/Casting.h"<br>
+#include "llvm/Support/ManagedStatic.h"<br>
+#include "llvm/Support/Mutex.h"<br>
<br>
namespace llvm {<br>
class Function;<br>
@@ -35,6 +37,7 @@ class LLVMContext;<br>
class Metadata;<br>
class MDTuple;<br>
class MDNode;<br>
+class Module;<br>
<br>
inline const char *getHotSectionPrefix() { return ".hot"; }<br>
inline const char *getUnlikelySectionPrefix() { return ".unlikely"; }<br>
@@ -67,6 +70,14 @@ private:<br>
// appears in the profile. The map is kept sorted in the descending order of<br>
// counts.<br>
std::map<uint64_t, uint32_t, std::greater<uint64_t>> CountFrequencies;<br>
+ // Compute profile summary for a module.<br>
+ static ProfileSummary *computeProfileSummary(Module *M);<br>
+ // Cache of last seen module and its profile summary.<br>
+ static ManagedStatic<std::pair<Module *, std::unique_ptr<ProfileSummary>>><br>
+ CachedSummary;<br>
+ // Mutex to access summary cache<br>
+ static ManagedStatic<sys::SmartMutex<true>> CacheMutex;<br></blockquote><div><br></div></div></div><div>Hi Easwaran,</div><div><br></div><div>In the LLVM core libraries, we don't use globals like this.</div><div><br></div><div>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.</div><div><br></div><div>Thanks,</div><div><br></div><div>-- Sean Silva</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
protected:<br>
SummaryEntryVector DetailedSummary;<br>
std::vector<uint32_t> DetailedSummaryCutoffs;<br>
@@ -85,14 +96,12 @@ protected:<br>
: PSK(K), DetailedSummary(DetailedSummary), TotalCount(TotalCount),<br>
MaxCount(MaxCount), MaxFunctionCount(MaxFunctionCount),<br>
NumCounts(NumCounts), NumFunctions(NumFunctions) {}<br>
- ~ProfileSummary() = default;<br>
inline void addCount(uint64_t Count);<br>
/// \brief Return metadata specific to the profile format.<br>
/// Derived classes implement this method to return a vector of Metadata.<br>
virtual std::vector<Metadata *> getFormatSpecificMD(LLVMContext &Context) = 0;<br>
/// \brief Return detailed summary as metadata.<br>
Metadata *getDetailedSummaryMD(LLVMContext &Context);<br>
-<br>
public:<br>
static const int Scale = 1000000;<br>
Kind getKind() const { return PSK; }<br>
@@ -111,6 +120,10 @@ public:<br>
static ProfileSummary *getFromMD(Metadata *MD);<br>
uint32_t getNumFunctions() { return NumFunctions; }<br>
uint64_t getMaxFunctionCount() { return MaxFunctionCount; }<br>
+ /// \brief Get profile summary associated with module \p M<br>
+ static inline ProfileSummary *getProfileSummary(Module *M);<br>
+ virtual ~ProfileSummary() = default;<br>
+ virtual bool operator==(ProfileSummary &Other);<br>
};<br>
<br>
class InstrProfSummary final : public ProfileSummary {<br>
@@ -140,6 +153,7 @@ public:<br>
uint64_t getTotalCount() { return TotalCount; }<br>
uint64_t getMaxBlockCount() { return MaxCount; }<br>
uint64_t getMaxInternalBlockCount() { return MaxInternalBlockCount; }<br>
+ bool operator==(ProfileSummary &Other) override;<br>
};<br>
<br>
class SampleProfileSummary final : public ProfileSummary {<br>
@@ -180,5 +194,24 @@ SummaryEntryVector &ProfileSummary::getD<br>
return DetailedSummary;<br>
}<br>
<br>
+ProfileSummary *ProfileSummary::getProfileSummary(Module *M) {<br>
+ if (!M)<br>
+ return nullptr;<br>
+ sys::SmartScopedLock<true> Lock(*CacheMutex);<br>
+ // Computing profile summary for a module involves parsing a fairly large<br>
+ // metadata and could be expensive. We use a simple cache of the last seen<br>
+ // module and its profile summary.<br>
+ if (CachedSummary->first != M) {<br>
+ auto *Summary = computeProfileSummary(M);<br>
+ // Do not cache if the summary is empty. This is because a later pass<br>
+ // (sample profile loader, for example) could attach the summary metadata on<br>
+ // the module.<br>
+ if (!Summary)<br>
+ return nullptr;<br>
+ CachedSummary->first = M;<br>
+ CachedSummary->second.reset(Summary);<br>
+ }<br>
+ return CachedSummary->second.get();<br>
+}<br>
} // end namespace llvm<br>
#endif<br>
<br>
Modified: llvm/trunk/lib/Analysis/InlineCost.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/InlineCost.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/InlineCost.cpp Tue May 10 17:03:23 2016<br>
@@ -30,6 +30,7 @@<br>
#include "llvm/IR/InstVisitor.h"<br>
#include "llvm/IR/IntrinsicInst.h"<br>
#include "llvm/IR/Operator.h"<br>
+#include "llvm/ProfileData/ProfileCommon.h"<br>
#include "llvm/Support/Debug.h"<br>
#include "llvm/Support/raw_ostream.h"<br>
<br>
@@ -630,10 +631,11 @@ void CallAnalyzer::updateThreshold(CallS<br>
// a well-tuned heuristic based on *callsite* hotness and not callee hotness.<br>
uint64_t FunctionCount = 0, MaxFunctionCount = 0;<br>
bool HasPGOCounts = false;<br>
- if (Callee.getEntryCount() && Callee.getParent()->getMaximumFunctionCount()) {<br>
+ ProfileSummary *PS = ProfileSummary::getProfileSummary(Callee.getParent());<br>
+ if (Callee.getEntryCount() && PS) {<br>
HasPGOCounts = true;<br>
FunctionCount = Callee.getEntryCount().getValue();<br>
- MaxFunctionCount = Callee.getParent()->getMaximumFunctionCount().getValue();<br>
+ MaxFunctionCount = PS->getMaxFunctionCount();<br>
}<br>
<br>
// Listen to the inlinehint attribute or profile based hotness information<br>
<br>
Modified: llvm/trunk/lib/Analysis/LLVMBuild.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LLVMBuild.txt?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LLVMBuild.txt?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/LLVMBuild.txt (original)<br>
+++ llvm/trunk/lib/Analysis/LLVMBuild.txt Tue May 10 17:03:23 2016<br>
@@ -19,4 +19,4 @@<br>
type = Library<br>
name = Analysis<br>
parent = Libraries<br>
-required_libraries = Core Support<br>
+required_libraries = Core ProfileData Support<br>
<br>
Modified: llvm/trunk/lib/ProfileData/ProfileSummary.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/ProfileSummary.cpp?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/ProfileSummary.cpp?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/ProfileSummary.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/ProfileSummary.cpp Tue May 10 17:03:23 2016<br>
@@ -15,6 +15,7 @@<br>
#include "llvm/IR/Constants.h"<br>
#include "llvm/IR/Function.h"<br>
#include "llvm/IR/Metadata.h"<br>
+#include "llvm/IR/Module.h"<br>
#include "llvm/IR/Type.h"<br>
#include "llvm/ProfileData/InstrProf.h"<br>
#include "llvm/ProfileData/ProfileCommon.h"<br>
@@ -32,6 +33,10 @@ const std::vector<uint32_t> ProfileSumma<br>
900000, 950000, 990000, 999000, 999900, 999990, 999999});<br>
const char *ProfileSummary::KindStr[2] = {"InstrProf", "SampleProfile"};<br>
<br>
+ManagedStatic<std::pair<Module *, std::unique_ptr<ProfileSummary>>><br>
+ ProfileSummary::CachedSummary;<br>
+ManagedStatic<sys::SmartMutex<true>> ProfileSummary::CacheMutex;<br>
+<br>
void InstrProfSummary::addRecord(const InstrProfRecord &R) {<br>
// The first counter is not necessarily an entry count for IR<br>
// instrumentation profiles.<br>
@@ -86,6 +91,39 @@ void ProfileSummary::computeDetailedSumm<br>
}<br>
}<br>
<br>
+bool ProfileSummary::operator==(ProfileSummary &Other) {<br>
+ if (getKind() != Other.getKind())<br>
+ return false;<br>
+ if (TotalCount != Other.TotalCount)<br>
+ return false;<br>
+ if (MaxCount != Other.MaxCount)<br>
+ return false;<br>
+ if (MaxFunctionCount != Other.MaxFunctionCount)<br>
+ return false;<br>
+ if (NumFunctions != Other.NumFunctions)<br>
+ return false;<br>
+ if (NumCounts != Other.NumCounts)<br>
+ return false;<br>
+ std::vector<ProfileSummaryEntry> DS1 = getDetailedSummary();<br>
+ std::vector<ProfileSummaryEntry> DS2 = Other.getDetailedSummary();<br>
+ auto CompareSummaryEntry = [](ProfileSummaryEntry &E1,<br>
+ ProfileSummaryEntry &E2) {<br>
+ return E1.Cutoff == E2.Cutoff && E1.MinCount == E2.MinCount &&<br>
+ E1.NumCounts == E2.NumCounts;<br>
+ };<br>
+ if (!std::equal(DS1.begin(), DS1.end(), DS2.begin(), CompareSummaryEntry))<br>
+ return false;<br>
+ return true;<br>
+}<br>
+<br>
+bool InstrProfSummary::operator==(ProfileSummary &Other) {<br>
+ InstrProfSummary *OtherIPS = dyn_cast<InstrProfSummary>(&Other);<br>
+ if (!OtherIPS)<br>
+ return false;<br>
+ return MaxInternalBlockCount == OtherIPS->MaxInternalBlockCount &&<br>
+ ProfileSummary::operator==(Other);<br>
+}<br>
+<br>
// Returns true if the function is a hot function.<br>
bool ProfileSummary::isFunctionHot(const Function *F) {<br>
// FIXME: update when summary data is stored in module's metadata.<br>
@@ -366,3 +404,9 @@ ProfileSummary *ProfileSummary::getFromM<br>
else<br>
return nullptr;<br>
}<br>
+<br>
+ProfileSummary *ProfileSummary::computeProfileSummary(Module *M) {<br>
+ if (Metadata *MD = M->getProfileSummary())<br>
+ return getFromMD(MD);<br>
+ return nullptr;<br>
+}<br>
<br>
Modified: llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll (original)<br>
+++ llvm/trunk/test/Transforms/Inline/inline-cold-callee.ll Tue May 10 17:03:23 2016<br>
@@ -5,7 +5,7 @@<br>
; A callee with identical body does gets inlined because cost fits within the<br>
; inline-threshold<br>
<br>
-define i32 @callee1(i32 %x) !prof !1 {<br>
+define i32 @callee1(i32 %x) !prof !21 {<br>
%x1 = add i32 %x, 1<br>
%x2 = add i32 %x1, 1<br>
%x3 = add i32 %x2, 1<br>
@@ -13,7 +13,7 @@ define i32 @callee1(i32 %x) !prof !1 {<br>
ret i32 %x3<br>
}<br>
<br>
-define i32 @callee2(i32 %x) !prof !2 {<br>
+define i32 @callee2(i32 %x) !prof !22 {<br>
; CHECK-LABEL: @callee2(<br>
%x1 = add i32 %x, 1<br>
%x2 = add i32 %x1, 1<br>
@@ -22,7 +22,7 @@ define i32 @callee2(i32 %x) !prof !2 {<br>
ret i32 %x3<br>
}<br>
<br>
-define i32 @caller2(i32 %y1) !prof !2 {<br>
+define i32 @caller2(i32 %y1) !prof !22 {<br>
; CHECK-LABEL: @caller2(<br>
; CHECK: call i32 @callee2<br>
; CHECK-NOT: call i32 @callee1<br>
@@ -32,8 +32,19 @@ define i32 @caller2(i32 %y1) !prof !2 {<br>
ret i32 %y3<br>
}<br>
<br>
-!llvm.module.flags = !{!0}<br>
-!0 = !{i32 1, !"MaxFunctionCount", i32 1000}<br>
-!1 = !{!"function_entry_count", i64 100}<br>
-!2 = !{!"function_entry_count", i64 1}<br>
-<br>
+!llvm.module.flags = !{!1}<br>
+!21 = !{!"function_entry_count", i64 100}<br>
+!22 = !{!"function_entry_count", i64 1}<br>
+<br>
+!1 = !{i32 1, !"ProfileSummary", !2}<br>
+!2 = !{!3, !4, !5, !6, !7, !8, !9, !10}<br>
+!3 = !{!"ProfileFormat", !"InstrProf"}<br>
+!4 = !{!"TotalCount", i64 10000}<br>
+!5 = !{!"MaxBlockCount", i64 1000}<br>
+!6 = !{!"MaxInternalBlockCount", i64 1}<br>
+!7 = !{!"MaxFunctionCount", i64 1000}<br>
+!8 = !{!"NumBlocks", i64 3}<br>
+!9 = !{!"NumFunctions", i64 3}<br>
+!10 = !{!"DetailedSummary", !11}<br>
+!11 = !{!12}<br>
+!12 = !{i32 10000, i64 0, i32 0}<br>
<br>
Modified: llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll (original)<br>
+++ llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll Tue May 10 17:03:23 2016<br>
@@ -5,7 +5,7 @@<br>
; A cold callee with identical body does not get inlined because cost exceeds the<br>
; inline-threshold<br>
<br>
-define i32 @callee1(i32 %x) !prof !1 {<br>
+define i32 @callee1(i32 %x) !prof !20 {<br>
%x1 = add i32 %x, 1<br>
%x2 = add i32 %x1, 1<br>
%x3 = add i32 %x2, 1<br>
@@ -13,7 +13,7 @@ define i32 @callee1(i32 %x) !prof !1 {<br>
ret i32 %x3<br>
}<br>
<br>
-define i32 @callee2(i32 %x) !prof !2 {<br>
+define i32 @callee2(i32 %x) !prof !21 {<br>
; CHECK-LABEL: @callee2(<br>
%x1 = add i32 %x, 1<br>
%x2 = add i32 %x1, 1<br>
@@ -22,7 +22,7 @@ define i32 @callee2(i32 %x) !prof !2 {<br>
ret i32 %x3<br>
}<br>
<br>
-define i32 @caller2(i32 %y1) !prof !2 {<br>
+define i32 @caller2(i32 %y1) !prof !21 {<br>
; CHECK-LABEL: @caller2(<br>
; CHECK: call i32 @callee2<br>
; CHECK-NOT: call i32 @callee1<br>
@@ -32,8 +32,19 @@ define i32 @caller2(i32 %y1) !prof !2 {<br>
ret i32 %y3<br>
}<br>
<br>
-!llvm.module.flags = !{!0}<br>
-!0 = !{i32 1, !"MaxFunctionCount", i32 10}<br>
-!1 = !{!"function_entry_count", i64 10}<br>
-!2 = !{!"function_entry_count", i64 1}<br>
-<br>
+!llvm.module.flags = !{!1}<br>
+!20 = !{!"function_entry_count", i64 10}<br>
+!21 = !{!"function_entry_count", i64 1}<br>
+<br>
+!1 = !{i32 1, !"ProfileSummary", !2}<br>
+!2 = !{!3, !4, !5, !6, !7, !8, !9, !10}<br>
+!3 = !{!"ProfileFormat", !"InstrProf"}<br>
+!4 = !{!"TotalCount", i64 10000}<br>
+!5 = !{!"MaxBlockCount", i64 10}<br>
+!6 = !{!"MaxInternalBlockCount", i64 1}<br>
+!7 = !{!"MaxFunctionCount", i64 10}<br>
+!8 = !{!"NumBlocks", i64 3}<br>
+!9 = !{!"NumFunctions", i64 3}<br>
+!10 = !{!"DetailedSummary", !11}<br>
+!11 = !{!12}<br>
+!12 = !{i32 10000, i64 0, i32 0}<br>
<br>
Modified: llvm/trunk/unittests/ProfileData/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/CMakeLists.txt?rev=269131&r1=269130&r2=269131&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/CMakeLists.txt?rev=269131&r1=269130&r2=269131&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/ProfileData/CMakeLists.txt (original)<br>
+++ llvm/trunk/unittests/ProfileData/CMakeLists.txt Tue May 10 17:03:23 2016<br>
@@ -8,5 +8,6 @@ set(LLVM_LINK_COMPONENTS<br>
add_llvm_unittest(ProfileDataTests<br>
CoverageMappingTest.cpp<br>
InstrProfTest.cpp<br>
+ ProfileSummaryTest.cpp<br>
SampleProfTest.cpp<br>
)<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>