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

Easwaran Raman via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 10 14:30:16 PST 2015


eraman marked 6 inline comments as done.

================
Comment at: include/llvm/Analysis/InlineCost.h:145
@@ +144,3 @@
+/// \brief Compute threshold for a given optimization level.
+int computeThresholdFromOptLevels(unsigned OptLevel, unsigned SizeOptLevel);
+
----------------
davidxl wrote:
> 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?
I initially had it as a static member function, but the thought of calling this inside CallAnalyzer (not in this patch, but when replacing the constant threshold for indirect call bonus) didn't feel right to me (CallAnalyzer being 'aware' of InlineCostAnalysis). Functionally there is no difference and if you strongly prefer  I'll make it a static member function.

================
Comment at: lib/Analysis/InlineCost.cpp:1385
@@ +1384,3 @@
+
+static cl::opt<int> InlineLimit(
+    "inline-threshold", cl::Hidden, cl::init(225), cl::ZeroOrMore,
----------------
davidxl wrote:
> Change it to DefaultInlineThreshold?
I don't really like the name, but went ahead with it. Of course we don't want to change the option itself.

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
davidxl wrote:
> davidxl wrote:
> > 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.
> Why do we need a threshold parameter here? The base threshold should be the default threshold.
The idea of calling a  method of InlineCostAnalysis from this inside CallAnalyzer (not in this patch, but when replacing the constant threshold for indirect call bonus) doesn't feel right to me. Functionally there is no difference and if you strongly prefer I'll make it a static member function.

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
eraman wrote:
> davidxl wrote:
> > davidxl wrote:
> > > 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.
> > Why do we need a threshold parameter here? The base threshold should be the default threshold.
> The idea of calling a  method of InlineCostAnalysis from this inside CallAnalyzer (not in this patch, but when replacing the constant threshold for indirect call bonus) doesn't feel right to me. Functionally there is no difference and if you strongly prefer I'll make it a static member function.
Since there is only one caller, I haven't added the wrapper.

================
Comment at: lib/Analysis/InlineCost.cpp:1414
@@ +1413,3 @@
+
+static int getInlineThreshold(CallSite CS, int CallSiteIndependentThreshold) {
+
----------------
eraman wrote:
> eraman wrote:
> > davidxl wrote:
> > > davidxl wrote:
> > > > 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.
> > > Why do we need a threshold parameter here? The base threshold should be the default threshold.
> > The idea of calling a  method of InlineCostAnalysis from this inside CallAnalyzer (not in this patch, but when replacing the constant threshold for indirect call bonus) doesn't feel right to me. Functionally there is no difference and if you strongly prefer I'll make it a static member function.
> Since there is only one caller, I haven't added the wrapper.
I am not sure what you mean by the base threshold. You pass a threshold that is not dependent on the callsite. This may get changed based on callsite properties and returned.

================
Comment at: lib/Analysis/InlineCost.cpp:1421
@@ +1420,3 @@
+  bool OptSize = Caller && !Caller->isDeclaration() &&
+                 // FIXME: Use Function::optForSize().
+                 Caller->hasFnAttribute(Attribute::OptimizeForSize);
----------------
hfinkel wrote:
> Seems like we could fix this FIXME while we're here.
> 
If the function has a MinSize attribute, this OptSize bool will be false, but I think that is not intentional. If I want a NFC patch, I should replace Caller->hasFnAttribute(Attribute::OptimizeForSize) with  (Caller->optForSize() && ! Caller->optForMinSize()) to replicate the exact behavior. Instead I thought I'll just replace it with  Caller->optForSize() as a separate patch (or perhaps I am obsessing a lot over keeping this NFC since this not using a smaller threshold for MinSize attribute seems unintentional)

================
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);
   }
----------------
davidxl wrote:
> This does not look right --  DefaultThreshold is used here which should not be.
Why? the Threshold could be either the default one or based on opt levels. This is the callsite independent threshold which is modified by getInlineCost


Repository:
  rL LLVM

http://reviews.llvm.org/D15401





More information about the llvm-commits mailing list