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

[David Li via llvm-commits]

davidxl added a comment.

Threshold is used in inline cost analysis to bail out early for compile time reasons, so it is an implementation detail that should not be exposed at interface level. Currently it is needed to be passed because the DefaultThreshold (aka callSiteIndependent Threshold) depends on opt level, sizeOptLevel which ICA does not have access to.

The solution is pretty simple:  just pass the optlevel to ICA when ICA is created.   If Threshold does need to be passed for some reason (which I doubt) please rename such parameters (including the member field in SimpleInliner) to be DefaultThreshold (with comment it depends on opt level) to avoid confusion. CallsiteIndependentThreshold is too long a name to use.


================
Comment at: include/llvm/Analysis/InlineCost.h:145
@@ +144,3 @@
+/// \brief Compute threshold for a given optimization level.
+int computeThresholdFromOptLevels(unsigned OptLevel, unsigned SizeOptLevel);
+
----------------
eraman wrote:
> 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.
SizeOptLevel == 1 maps to -Os, while SizeOptLevel == 2 maps to -Oz.  In fact, FE also sets OptimizeForSize and MinSize function attribute when the command line option is on:

 if (!HasOptnone) {
    if (CodeGenOpts.OptimizeSize)
      FuncAttrs.addAttribute(llvm::Attribute::OptimizeForSize);
    if (CodeGenOpts.OptimizeSize == 2)
      FuncAttrs.addAttribute(llvm::Attribute::MinSize);
  }

This means inliner SizeOptLevel is not needed here. 

================
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);
   }
----------------
eraman wrote:
> 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
Ok -- I see that the getInlineThreshold call is pushed into getInlineCost method so this is correct.


Repository:
  rL LLVM

http://reviews.llvm.org/D15401