Use the inlinehint-threshold for hot callees.

[eraman]

Summary

This patch uses the higher inlinehint-threshold for callees that are considered hot. The frontend already uses the same hotness threshold (callee's entry count >= 30% of max function entry count) to set inline hints on such callees, which results in the inlinehint-threshold being used anyway, so there is no functional change in this patch. The idea is to remove that code that sets inline hints based on hotness from the frontend and make the PGO based inlining decisions in the inliner itself. Eventually what we really want is callsite based hotness thresholds, but that is a heuristic change that needs tuning and will be done in a separate patch.

[davidxl]

The title of the patch is wrong. It is not setting higher threshold but moving (part of it) the FE Inline hint logic to the inliner.

[davidxl]

The clean up looks ok to me (NFC at this point). I added Duncan to the list to see if there are any concerns.

[eraman]

Well, it does use a higher (when compared to the default) threshold, but
I've changed the title and description to make it more clear.

[dnovillo]

Once this is in, please update lib/Transforms/IPO/SampleProfile.cpp. It does something similar.

Also, should cold functions be marked with the cold hint?

[davidxl]

LGTM. As Justin says, please continue the clean up effort: 1) unify the profile summary data for PGO and Sample FDO 2) move the cold attribute logic to LLVM

[eraman]

1. Use the hint threshold only if it is greater than regular threshold.
2. Added a test case.

[eraman]

Duncan and Justin, does this patch look good?

[eraman]

I am now checking for cold calls and using the cold threshold as well and added a test case.

Earlier in this thread I had commented that Cold attribute is used in other places as well, but I was wrong. Branch probability computation checks for Cold attribute in instruction and not the function. With this change, both setting of cold and inlinehint attribute can be removed from CodeGenPGO.cpp in CFE. Does this look good?

[davidxl]

This looks very good to me (also addressed cold attribute feedback by Justin) -- and the test cases are nice. Ok to check in with the 'nits' fixed. Looking forward to the FE cleanup.

lib/Transforms/IPO/Inliner.cpp

319

I would suggest parametize 0.3 and 0.01 below, but since there is on-going work on profile summary, it is probably not necessary.

test/Transforms/Inline/inline-cold-callee.ll

36 ↗

(On Diff #43282)

nit -- please make MaxFunctionCount larger so that the test case is more stable to future tunings.

[eraman]

I'll address David's comment on test case (although I don't think it matters much) and if I don't hear from Justin and Duncan will submit this patch later today.

lib/Transforms/IPO/Inliner.cpp

unsigned Inliner::getInlineThreshold(CallSite CS) const {
   // Listen to the inlinehint attribute when it would increase the threshold
   // and the caller does not need to minimize its size.
   Function *Callee = CS.getCalledFunction();
   bool InlineHint = Callee && !Callee->isDeclaration() &&
                     Callee->hasFnAttribute(Attribute::InlineHint);
   if (InlineHint && HintThreshold > Threshold &&
       !Caller->hasFnAttribute(Attribute::MinSize))
     Threshold = HintThreshold;
+  // If profile information is available, use that to prioritize hot functions.
+  // FIXME: The heuristic used here is based on preliminary SPEC tuning and
+  // may not be optimal. Replace this with a well-tuned heuristic based on
+  // *callsite* hotness and not callee hotness.
+  auto EntryCount = Callee->getEntryCount();
+  auto MaxFunctionCount = Callee->getParent()->getMaximumFunctionCount();
+  if (HintThreshold > Threshold && EntryCount && MaxFunctionCount &&
+      EntryCount.getValue() >=
+          (uint64_t)(0.3 * (double)MaxFunctionCount.getValue())) {
+    Threshold = HintThreshold;
+  }
 
   // Listen to the cold attribute when it would decrease the threshold.
   bool ColdCallee = Callee && !Callee->isDeclaration() &&
                     Callee->hasFnAttribute(Attribute::Cold);
   // Command line argument for InlineLimit will override the default
   // ColdThreshold. If we have -inline-threshold but no -inlinecold-threshold,
   // do not use the default cold threshold even if it is smaller.
   if ((InlineLimit.getNumOccurrences() == 0 ||
        ColdThreshold.getNumOccurrences() > 0) && ColdCallee &&
       ColdThreshold < Threshold)
     Threshold = ColdThreshold;
 
   return Threshold;
 }
 
 static void emitAnalysis(CallSite CS, const Twine &Msg) {
   Function *Caller = CS.getCaller();
   LLVMContext &Ctx = Caller->getContext();

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

+; RUN: opt < %s -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
+
+define i32 @callee1(i32 %x) !prof !1 {
+  %x1 = add i32 %x, 1
+  %x2 = add i32 %x1, 1
+  %x3 = add i32 %x2, 1
+
+  ret i32 %x3
+}
+
+define i32 @callee2(i32 %x) !prof !2 {
+; CHECK-LABEL: @callee2(
+  %x1 = add i32 %x, 1
+  %x2 = add i32 %x1, 1
+  %x3 = add i32 %x2, 1
+
+  ret i32 %x3
+}
+
+define i32 @caller2(i32 %y1) !prof !2 {
+; CHECK-LABEL: @caller2(
+; CHECK: call i32 @callee2
+; CHECK-NOT: call i32 @callee1
+; CHECK: ret i32 %x3.i
+  %y2 = call i32 @callee2(i32 %y1)
+  %y3 = call i32 @callee1(i32 %y2)
+  ret i32 %y3
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"MaxFunctionCount", i32 10}
+!1 = !{!"function_entry_count", i64 10}
+!2 = !{!"function_entry_count", i64 1}
+