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

[Easwaran Raman via llvm-commits]

eraman marked 2 inline comments as done.

================
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:85
@@ +84,3 @@
+  ProfileSummaryAnalysis(ProfileSummaryAnalysis &&Arg) {}
+  ProfileSummaryAnalysis &operator=(const ProfileSummaryAnalysis &RHS) {
+    return *this;
----------------
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)

================
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:100
@@ +99,3 @@
+/// \brief Printer pass that uses \c ProfileSummaryAnalysis.
+class FunctionHotnessPrinterPass
+    : public PassInfoMixin<FunctionHotnessPrinterPass> {
----------------
davidxl wrote:
> Nit : ProfileSummaryPrinterPass?
I thought FunctionHotnessPrinterPass accurately reflects what this is doing. I have no strong opinion - will change it to ProfileSummaryPrinterPass if you prefer.

================
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();
----------------
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. 

================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:106
@@ +105,3 @@
+
+bool ProfileSummaryInfo::isHotCount(uint64_t C) {
+  if (!HotCountThreshold)
----------------
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. 


http://reviews.llvm.org/D20648