[PATCH] D15401: Refactor threshold computation for inline cost analysis

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Analysis/InlineCost.h:145
@@ +144,3 @@
+/// \brief Compute threshold for a given optimization level.
+int computeThresholdFromOptLevels(unsigned OptLevel, unsigned SizeOptLevel);
+
----------------
hfinkel wrote:
> Given that we don't really have a SizeOptLevel in LLVM, but -Os, -Oz settings, we should document here what SizeOptLevel means.
make it a static member function of InlineCostAnalysis?

================
Comment at: include/llvm/Analysis/InlineCost.h:148
@@ -141,1 +147,3 @@
+/// \brief Return the default value of -inline-threshold.
+int getDefaultInlineThreshold();
 }
----------------
Same here.

================
Comment at: lib/Analysis/InlineCost.cpp:1385
@@ +1384,3 @@
+
+static cl::opt<int> InlineLimit(
+    "inline-threshold", cl::Hidden, cl::init(225), cl::ZeroOrMore,
----------------
Change it to DefaultInlineThreshold?

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
Should it be a static member function of InlineCostAnalysis class?

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
davidxl wrote:
> Should it be a static member function of InlineCostAnalysis class?
Missing documentation of the method.

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
davidxl wrote:
> davidxl wrote:
> > Should it be a static member function of InlineCostAnalysis class?
> Missing documentation of the method.
The interface of this method also needs to be changed to take an additional callee function as the argument so that indirect callsite's threshold can be computed. 

The existing method can be a wrapper to this one.

================
Comment at: lib/Transforms/IPO/InlineSimple.cpp:41
@@ -40,2 +40,3 @@
   InlineCostAnalysis *ICA;
+  int Threshold;
 
----------------
-> DefaultThreshold

================
Comment at: lib/Transforms/IPO/InlineSimple.cpp:57
@@ -54,3 +56,3 @@
   InlineCost getInlineCost(CallSite CS) override {
-    return ICA->getInlineCost(CS, getInlineThreshold(CS));
+    return ICA->getInlineCost(CS, Threshold);
   }
----------------
This does not look right --  DefaultThreshold is used here which should not be.


Repository:
  rL LLVM

http://reviews.llvm.org/D15401