[PATCH] D22120: Move inline threshold related flags to InlineSimple.cpp

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: include/llvm/Analysis/InlineCost.h:38
@@ +37,3 @@
+
+// Use when -O[34] is specified.
+const int OptAggressiveThreshold = 275;
----------------
davidxl wrote:
> mehdi_amini wrote:
> > davidxl wrote:
> > > mehdi_amini wrote:
> > > > There is no O4, did you mean `O{1,2,3}` here?
> > > Clang driver maps O4 to O3 (so O4 is actually 'supported').
> > Sure, but that's a clang "compatibility option" (O4 use to trigger LTO on top of O3). On the LLVM side O4 does not exist (I'm not sure it has ever existed in LLVM).
> > That said, I don't really mind if you want to add O4, but clarify the situation for O1/O2.
> ok -- this should be cleaned up then (it was in the original code that got moved here).
> 
> By the way, why deprecating O4?  It is common to use it  to turn on LTO.
Because LTO requires a complete change to the compilation model -- it isn't just an optimization level.

Still further, the other optimization levels still make sense within that model. We might want -Oz, -Os, -O2, and -O3 very usefully (and with different results) when doing LTO.

I would keep O4 out of LLVM for clarity -- it is really only supposed to be a compatibility thing in Clang. We should probably mark it as deprecated or something as I switched the behavior a loooong time ago.

================
Comment at: include/llvm/Analysis/InlineCost.h:118
@@ +117,3 @@
+  /// The default threshold to start with for a callee.
+  int DefaultThreshold;
+  /// Threshold to use for callees with inline hint.
----------------
davidxl wrote:
> davidxl wrote:
> > I prefer this one be called 'InlineThreshold' (to not be confused with the internal option) and document how it is set.
> Or perhaps keeping this parameter name but changing the option name to be InlineThreshold (not Default...)
I like calling it "default" if it is OptSize is and such are going to override it.

Which parameter were you thinking that is currently named default?

================
Comment at: lib/Transforms/IPO/InlineSimple.cpp:34-48
@@ -33,2 +33,17 @@
 
+static cl::opt<int> DefaultInlineThreshold(
+    "inline-threshold", cl::Hidden, cl::init(225), cl::ZeroOrMore,
+    cl::desc("Control the amount of inlining to perform (default = 225)"));
+
+static cl::opt<int> HintThreshold(
+    "inlinehint-threshold", cl::Hidden, cl::init(325),
+    cl::desc("Threshold for inlining functions with inline hint"));
+
+// We introduce this threshold to help performance of instrumentation based
+// PGO before we actually hook up inliner with analysis passes such as BPI and
+// BFI.
+static cl::opt<int> ColdThreshold(
+    "inlinecold-threshold", cl::Hidden, cl::init(225),
+    cl::desc("Threshold for inlining functions with cold attribute"));
+
 namespace {
----------------
I would much prefer these command line flags remain in the InlineCost analysis and next to all the other numbers used to tune things.


https://reviews.llvm.org/D22120