[Inliner] Do not mix callsite and callee hotness based updates.

[eraman]

Summary

Update threshold based on callee's hotness only when BFI is not available. Otherwise use only callsite's hotness. This makes it easier to reason about hotness related threshold updates.

[chandlerc]

It would be good in the patch description to maybe clarify the intent / motivation here? I assume the goal is to simplify reasoning about which thresholds are triggered in which cases when the new PM is available? At least, that is my guess based on our in-person conversations.

Anyways, feel free to submit, just seems good to provide context to others reading.

test/Transforms/Inline/inline-hot-callsite.ll

2 ↗

(On Diff #98861)

; without ; without -> ; without

[eraman]

Turns out that this change is incorrect. When sample profile is used, we use callsite hotness even without BFI. In a separate patch, I plan to add a method in ProfileSummaryInfo to determine if the module has sample profile. Then I can fix this patch by using callsite hotness either when sample profile is used or when BFI is available.

[chandlerc]

(Just marking this as no longer ready to go so it shows back up in my queue...)

In D33157#756345, @eraman wrote:

Turns out that this change is incorrect. When sample profile is used, we use callsite hotness even without BFI. In a separate patch, I plan to add a method in ProfileSummaryInfo to determine if the module has sample profile. Then I can fix this patch by using callsite hotness either when sample profile is used or when BFI is available.

Good catch!!!

[eraman]

Use callsite's hotness for sample PGO.

[chandlerc]

LGTM again! =D

llvm/trunk/lib/Analysis/InlineCost.cpp

void CallAnalyzer::updateThreshold(CallSite CS, Function &Callee) {
 
   // Adjust the threshold based on inlinehint attribute and profile based
   // hotness information if the caller does not have MinSize attribute.
   if (!Caller->optForMinSize()) {
     if (Callee.hasFnAttribute(Attribute::InlineHint))
       Threshold = MaxIfValid(Threshold, Params.HintThreshold);
     if (PSI) {
       BlockFrequencyInfo *CallerBFI = GetBFI ? &((*GetBFI)(*Caller)) : nullptr;
+      // FIXME: After switching to the new passmanager, simplify the logic below
+      // by checking only the callsite hotness/coldness. The check for CallerBFI
+      // exists only because we do not have BFI available with the old PM.
+      //
+      // Use callee's hotness information only if we have no way of determining
+      // callsite's hotness information. Callsite hotness can be determined if
+      // sample profile is used (which adds hotness metadata to calls) or if
+      // caller's BlockFrequencyInfo is available.
+      if (CallerBFI || PSI->hasSampleProfile()) {
         if (PSI->isHotCallSite(CS, CallerBFI)) {
           DEBUG(dbgs() << "Hot callsite.\n");
           Threshold = Params.HotCallSiteThreshold.getValue();
-      } else if (PSI->isFunctionEntryHot(&Callee)) {
+        } else if (PSI->isColdCallSite(CS, CallerBFI)) {
+          DEBUG(dbgs() << "Cold callsite.\n");
+          Threshold = MinIfValid(Threshold, Params.ColdCallSiteThreshold);
+        }
+      } else {
+        if (PSI->isFunctionEntryHot(&Callee)) {
           DEBUG(dbgs() << "Hot callee.\n");
           // If callsite hotness can not be determined, we may still know
           // that the callee is hot and treat it as a weaker hint for threshold
           // increase.
           Threshold = MaxIfValid(Threshold, Params.HintThreshold);
-      } else if (PSI->isColdCallSite(CS, CallerBFI)) {
-        DEBUG(dbgs() << "Cold callsite.\n");
-        Threshold = MinIfValid(Threshold, Params.ColdCallSiteThreshold);
         } else if (PSI->isFunctionEntryCold(&Callee)) {
           DEBUG(dbgs() << "Cold callee.\n");
           Threshold = MinIfValid(Threshold, Params.ColdThreshold);
         }
       }
     }
+  }
 
   // Finally, take the target-specific inlining threshold multiplier into
   // account.
   Threshold *= TTI.getInliningThresholdMultiplier();
 }
 
 bool CallAnalyzer::visitCmpInst(CmpInst &I) {
   Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);

llvm/trunk/test/Transforms/Inline/inline-hot-callee.ll

 ; RUN: opt < %s -inline -inline-threshold=0 -inlinehint-threshold=100 -S | FileCheck %s
-; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -inline-threshold=0 -inlinehint-threshold=100 -S | FileCheck %s
 
-; This tests that a hot callee gets the (higher) inlinehint-threshold even without
-; inline hints and gets inlined because the cost is less than inlinehint-threshold.
-; A cold callee with identical body does not get inlined because cost exceeds the
-; inline-threshold
+; This tests that a hot callee gets the (higher) inlinehint-threshold even
+; without inline hints and gets inlined because the cost is less than
+; inlinehint-threshold. A cold callee with identical body does not get inlined
+; because cost exceeds the inline-threshold. This test is relevant only when the
+; old pass manager is used.
 
 define i32 @callee1(i32 %x) !prof !21 {
   %x1 = add i32 %x, 1
   %x2 = add i32 %x1, 1
   %x3 = add i32 %x2, 1
   call void @extern()
   ret i32 %x3
 }