Use isFunctionHotInCallGraph to set the function section prefix.

[danielcdh]

Summary

The current prefix based function layout algorithm only looks at function's entry count, which is not sufficient. A function should be grouped together if its entry count or any call edge count is hot.

[eraman]

I concur that the current state of checking the hotness of entry count is not good. I am not sure this is the right approach without seeing some numbers. Consider a function foo with calls to bar1 and bar2. Let's say foo's entry is cold. Neither foo->bar1 nor foo->bar2 is hot, but the sum of their counts is hot. Your new solution will still place foo in cold section. So may be we should add all outgoing counts and check its hotness. Even then, we may not want to put foo in cold section if the sum of outgoing edges is not cold.

[danielcdh]

update

[danielcdh]

Updated the patch to change for the cold prefix tool.

In terms of performance, it will only affect ITLB performance on very large applications. For the perf test I've done, it shows improved ITLB and heatmap, but no visible performance difference is observed.

[eraman]

lib/Analysis/ProfileSummaryInfo.cpp

128

Based on our offline conversation, I thought we want a function to be labeled only if both its entry count and the outgoing edge count sum is cold, no? With this code, a cold function can still have non-hot non-cold out edges.

I think it makes sense to have

coldInCallGraph => cold entry count AND cold out edges
hotInCallGraph => hot entry count OR hot out edges.

Do you agree?

[danielcdh]

update

[danielcdh]

lib/Analysis/ProfileSummaryInfo.cpp

128

You are right. Updated the code.

[eraman]

The code looks fine to me, but the test needs to be updated.

lib/Analysis/ProfileSummaryInfo.cpp

120

Nit: function's entry AND total call edge count is cold

121

Comment has been pasted from the previous function and hasn't been properly updated.

test/Transforms/CodeGenPrepare/section.ll

1

The test needs to be updated. You've added one case - a function with cold entry with hot call edge - but the other scenarios (sum of outgoing edges is hot, cold function with non-cold outgoing edges etc) are not tested.

[danielcdh]

update

[eraman]

LGTM after addressing the test changes.

test/Transforms/CodeGenPrepare/section.ll

1

It's better now, but I prefer you add cases where there are multiple out edges where sum of outgoing edges is hot (and individual edges are not) and sum of outgoing edges is not cold with individual edges being cold.

[danielcdh]

update

include/llvm/Analysis/ProfileSummaryInfo.h

public:
   ProfileSummaryInfo(Module &M) : M(M) {}
   ProfileSummaryInfo(ProfileSummaryInfo &&Arg)
       : M(Arg.M), Summary(std::move(Arg.Summary)) {}
   /// Returns the profile count for \p CallInst.
   static Optional<uint64_t> getProfileCount(const Instruction *CallInst,
                                             BlockFrequencyInfo *BFI);
   /// \brief Returns true if \p F has hot function entry.
   bool isFunctionEntryHot(const Function *F);
+  /// Returns true if \p F has hot function entry or hot call edge.
+  bool isFunctionHotInCallGraph(const Function *F);
   /// \brief Returns true if \p F has cold function entry.
   bool isFunctionEntryCold(const Function *F);
   /// \brief Returns true if \p F is a hot function.
   bool isHotCount(uint64_t C);
   /// \brief Returns true if count \p C is considered cold.
   bool isColdCount(uint64_t C);
   /// \brief Returns true if BasicBlock \p B is considered hot.
   bool isHotBB(const BasicBlock *B, BlockFrequencyInfo *BFI);

lib/Analysis/ProfileSummaryInfo.cpp

if (!F || !computeSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining hotness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
   return FunctionCount && isHotCount(FunctionCount.getValue());
 }
 
+/// Returns true if the function's entry or any of its call edge is hot.
+/// If it returns false, it either means it is not hot or it is unknown
+/// whether it is hot or not (for example, no profile data is available).
+bool ProfileSummaryInfo::isFunctionHotInCallGraph(const Function *F) {
+  if (!F || !computeSummary())
+    return false;
+  if (auto FunctionCount = F->getEntryCount())
+    if (isHotCount(FunctionCount.getValue()))
+      return true;
+
+  for (const auto &BB : *F)
+    for (const auto &I : BB)
+      if (isa<CallInst>(I) || isa<InvokeInst>(I))
+        if (auto CallCount = getProfileCount(&I, nullptr))
+          if (isHotCount(CallCount.getValue()))
+            return true;
+  return false;
+}
+
 /// Returns true if the function's entry is a cold. If it returns false, it
 /// either means it is not cold or it is unknown whether it is cold or not (for
 /// example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionEntryCold(const Function *F) {
   if (!F)
     return false;
   if (F->hasFnAttribute(Attribute::Cold))
     return true;
   if (!computeSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining coldness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
   return FunctionCount && isColdCount(FunctionCount.getValue());
 }
 

lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::runOnFunction(Function &F) {
   TLInfo = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
   TTI = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
   LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
   OptSize = F.optForSize();
 
   if (ProfileGuidedSectionPrefix) {
     ProfileSummaryInfo *PSI =
         getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
-    if (PSI->isFunctionEntryHot(&F))
+    if (PSI->isFunctionHotInCallGraph(&F))
       F.setSectionPrefix(".hot");
     else if (PSI->isFunctionEntryCold(&F))
       F.setSectionPrefix(".cold");
   }
 
   /// This optimization identifies DIV instructions that can be
   /// profitably bypassed and carried out with a shorter, faster divide.
   if (!OptSize && TLI && TLI->isSlowDivBypassed()) {

test/Transforms/CodeGenPrepare/section.ll

 ; RUN: opt < %s -codegenprepare -S | FileCheck %s
 
 target triple = "x86_64-pc-linux-gnu"
 
 ; This tests that hot/cold functions get correct section prefix assigned
 
 ; CHECK: hot_func{{.*}}!section_prefix ![[HOT_ID:[0-9]+]]
 define void @hot_func() !prof !15 {
   ret void
 }
 
+; CHECK: hot_call_func{{.*}}!section_prefix ![[HOT_ID]]
+define void @hot_call_func() !prof !16 {
+  call void @hot_func(), !prof !17
+  ret void
+}
+
 ; CHECK: cold_func{{.*}}!section_prefix ![[COLD_ID:[0-9]+]]
 define void @cold_func() !prof !16 {
   ret void
 }
 
 ; CHECK: ![[HOT_ID]] = !{!"function_section_prefix", !".hot"}
 ; CHECK: ![[COLD_ID]] = !{!"function_section_prefix", !".cold"}
 !llvm.module.flags = !{!1}
 !1 = !{i32 1, !"ProfileSummary", !2}
 !2 = !{!3, !4, !5, !6, !7, !8, !9, !10}
 !3 = !{!"ProfileFormat", !"InstrProf"}
 !4 = !{!"TotalCount", i64 10000}
 !5 = !{!"MaxCount", i64 1000}
 !6 = !{!"MaxInternalCount", i64 1}
 !7 = !{!"MaxFunctionCount", i64 1000}
 !8 = !{!"NumCounts", i64 3}
 !9 = !{!"NumFunctions", i64 3}
 !10 = !{!"DetailedSummary", !11}
 !11 = !{!12, !13, !14}
 !12 = !{i32 10000, i64 100, i32 1}
 !13 = !{i32 999000, i64 100, i32 1}
 !14 = !{i32 999999, i64 1, i32 2}
 !15 = !{!"function_entry_count", i64 1000}
 !16 = !{!"function_entry_count", i64 1}
+!17 = !{!"branch_weights", i32 1000}