[PATCH] D16381: Adjust inlining thresholds based on callsite hotness

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerPass.h:86
@@ +85,3 @@
+  // Inliner override them to update analysis information.
+  virtual BlockCloningFunctor getBlockCloningFunctor(CallSite &CS) {
+    return nullptr;
----------------
Can they be made pure virtual -- probably not if the derived class is not required to override it.

================
Comment at: include/llvm/Transforms/Utils/Cloning.h:51
@@ -50,1 +50,3 @@
 
+typedef std::function<void(const BasicBlock *, const BasicBlock *)>
+    BlockCloningFunctor;
----------------
Is there a common header to put this decl in?

================
Comment at: lib/Analysis/InlineCost.cpp:81
@@ -68,1 +80,3 @@
+                         cl::desc("Threshold for hot callsites "));
+
 namespace {
----------------
I suggest removing these two parameters in these patch, as they are for not irrelevant and confusing (and subject to change in the very near future when summary data is available to be used here).

The objective of this patch is to provide infrastructure hooks to feed profile data to the inliner, not to tune/or change the current inline behavior/decision -- as that needs more design and experiment.  To accomplish that goal, I suggest simply add a dummy wrapper function to do this:

int getAdjustedThreshold(int current_threshold, uint64_t callsite_count __attribute__((unused))) {
  // just return the input threshold for now
  return current_threshold;
}

In the future, I expect the threshold is different based on callsite_count and summary cutoffs.


================
Comment at: lib/Analysis/InlineCost.cpp:84
@@ -69,1 +83,3 @@
 
+const int ColdCallSiteCountThreshold = 10;
+
----------------
Remove this  -- it can be folded in the threshold adjusting method tuned in the future.

================
Comment at: lib/Analysis/InlineCost.cpp:624
@@ +623,3 @@
+  if (HasPGOCounts) {
+    uint64_t CallerEntryFreq = CallerBFI->getEntryFreq();
+    uint64_t CallSiteFreq = CallerBFI->getBlockFreq(CallBB).getFrequency();
----------------
Define a helper method to get callsite profile count.

================
Comment at: lib/Analysis/InlineCost.cpp:630
@@ +629,3 @@
+          CallerEntryCount.getValue() * CallSiteFreq / CallerEntryFreq;
+      if (CallSiteCount * 100 >= HotCallSitePercent * MaxFunctionCount)
+        Threshold = HotCallSiteThreshold;
----------------
call the proposed 'getAdjustedThreshold(Threshold, CallSiteCount);

================
Comment at: lib/Analysis/InlineCost.cpp:1467
@@ +1466,3 @@
+
+bool llvm::IsColdCallSite(CallSite CS, BlockFrequencyAnalysis *BFA) {
+  Function *Caller = CS.getCaller();
----------------
junbuml wrote:
> I cannot see any use of this function. Are you planing to hook this function in heuristic in this patch ? 
Remove unused function.


http://reviews.llvm.org/D16381