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

Easwaran Raman via llvm-commits llvm-commits at lists.llvm.org
Thu May 26 14:40:34 PDT 2016

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. 


More information about the llvm-commits mailing list