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

[Easwaran Raman via llvm-commits]

eraman added a comment.

In http://reviews.llvm.org/D16381#334500, @davidxl wrote:

> When the invalidation happens, the updated entry count information gets discarded it seems. This is wrong. The data should be propagated back to the IR. The information can be useful later for decisions such as function layout and optimize for size etc.


UpdateEntryCount in InlineSimple.cpp calls setEntryCount and hence the counts are not lost.

> Some test case is also needed to test that the entry counts can be properly updated during inlining.


Added a test case.


================
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;
----------------
davidxl wrote:
> Can they be made pure virtual -- probably not if the derived class is not required to override it.
AlwaysInliner then has to implement this method to return nullptr. I don't see any advantage in making it pyure virtual.

================
Comment at: include/llvm/Transforms/Utils/Cloning.h:51
@@ -50,1 +50,3 @@
 
+typedef std::function<void(const BasicBlock *, const BasicBlock *)>
+    BlockCloningFunctor;
----------------
davidxl wrote:
> Is there a common header to put this decl in?
I could leave it in Cloning.h and include it in InlineSimple.cpp and get rid of it in InlinerPass.h, but this adds an unnecessary dependence to InlineSimple.cpp. There is no other header file common to the inliner proper  and CloneFunction.cpp where it is appropriate to add this.

================
Comment at: lib/Analysis/InlineCost.cpp:1467
@@ +1466,3 @@
+
+bool llvm::IsColdCallSite(CallSite CS, BlockFrequencyAnalysis *BFA) {
+  Function *Caller = CS.getCaller();
----------------
davidxl wrote:
> 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.
Yes, I was planning to use this as part of tuning, but I've now removed it from this patch. 

================
Comment at: lib/Transforms/IPO/InlineSimple.cpp:175
@@ +174,3 @@
+  uint64_t CallerEntryFreq = CallerBFI->getEntryFreq();
+  uint64_t CallSiteCount = CallerCount.getValue() *
+                           (CallSiteFreq + CallerEntryFreq / 2) /
----------------
I have placed this helper in Inlinecost.cpp.


http://reviews.llvm.org/D16381