[PATCH] D20648: Analysis pass to access profile summary info

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:85
@@ +84,3 @@
+  ProfileSummaryAnalysis(ProfileSummaryAnalysis &&Arg) {}
+  ProfileSummaryAnalysis &operator=(const ProfileSummaryAnalysis &RHS) {
+    return *this;
----------------
eraman wrote:
> davidxl wrote:
> > Are those ctors/ assignment operators needed? The result type needs the rvalue ctors for data moving.
> This boilerplate code is found in many places. For example, see AssumptionAnalysis which also doesn't have any data members has this. From comments elsewhere, not specifying these breaks in MSVC (search for MSVC under include/llvm/Analysis)
ok.

================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:65
@@ +64,3 @@
+// Returns true if the function is a hot function.
+bool ProfileSummaryInfo::isHotFunction(const Function *F) {
+  computeSummary();
----------------
eraman wrote:
> davidxl wrote:
> > Should define a enum : SUMMARY_hot, _cold, _unknown (or noprofile)
> > 
> > We probably also need another interface in the future to check body hotness.
> I feel separate isHotX and isColdX is better. You anyway have to check the return value of the enum and the implementation has the combined code of isHotX and isColdX. But if you insist on unifying them, do you have any suggestion for "neither hot nor cold"? Unknown is not the right value. 
Ok -- document that when returns false, it either means it is not hot or we don't have info to tell.

================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:106
@@ +105,3 @@
+
+bool ProfileSummaryInfo::isHotCount(uint64_t C) {
+  if (!HotCountThreshold)
----------------
eraman wrote:
> davidxl wrote:
> > Document with FIXME that the hotness determination here is different from function hotness logic above.
> The fixme is in the is{Hot|Cold}Function since that is the one that needs to be fixed. 
ok


http://reviews.llvm.org/D20648