[PATCH] D24338: [InlineCost] Remove CallPenalty and change MinSizeThreshold to 5

[David Li via llvm-commits]

davidxl added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:188
+  /// and InlineCost changed to delegate to getCallCost.
+  int getInliningCallPenalty() const;
+  
----------------
I think it is a mistake to conflate two different things here. One is the penalty of 'not' inlining a callsite (aka the benefit of inlining it). This penalty models the branch(call)/return, function prologue and epilogue etc. It can also model the size impact of call instruction. The other is the overhead of a newly exposed callsite from the inline instance. The latter models the potential caller save register spills etc.  The former is the one that needs to defined in TTI.  For the latter, I think it should stay in Inliner proper. In the future, it can be replaced by target dependent register pressure analysis.  In other words, this interface should be getCallPenalty().  


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:132
   unsigned getInliningThresholdMultiplier() { return 1; }
+  unsigned getInliningCallPenalty() { return 5 * TTI::TCC_Basic; }
 
----------------
Why does it need to be tied with TCC_Basic?  Why not just defining an independent default?


================
Comment at: lib/Analysis/InlineCost.cpp:110
   int VectorBonus;
+  int CallPenalty;
 
----------------
This is a good parameter to be added to InlineParams struct


================
Comment at: lib/Analysis/InlineCost.cpp:943
       if (!isa<InlineAsm>(CS.getCalledValue()))
-        Cost += InlineConstants::CallPenalty;
+        Cost += CallPenalty;
     }
----------------
This one should use a different parameter from CallPenalty (that models overhead of exposed callsites -- which does not need to live in TTI)


================
Comment at: lib/Analysis/InlineCost.cpp:1206
+  // Cache the call penalty, adjusting based on command line flags if needed.
+  if (InliningCallPenalty != -1)
+    CallPenalty = InliningCallPenalty;
----------------
A more common way is to check getNumOfOccurences.


================
Comment at: lib/Analysis/InlineCost.cpp:1207
+  if (InliningCallPenalty != -1)
+    CallPenalty = InliningCallPenalty;
+  else
----------------
The computation here should be pushed to getInlineParams


================
Comment at: lib/Analysis/InlineCost.cpp:1209
+  else
+    CallPenalty = (TTI.getInliningCallPenalty() * InlineConstants::InstrCost) /
+                  TargetTransformInfo::TCC_Basic;
----------------
Why not directly using the TTI returned value?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:288
   // The candidate cost to be imposed upon the current function.
-  int CandidateCost = IC.getCost() - (InlineConstants::CallPenalty + 1);
+  int CandidateCost = IC.getCost() - 1;
   // This bool tracks what happens if we do NOT inline C into B.
----------------
should -1 be removed too?


Repository:
  rL LLVM

https://reviews.llvm.org/D24338