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

[Easwaran Raman via llvm-commits]

eraman marked 4 inline comments as done.
eraman added a comment.

In https://reviews.llvm.org/D22120#501569, @silvas wrote:

> To clarify, this patch itself is just a NFC refactoring in preparation for the real change, right? (to prevent the existing cl::opt flags from interfering with preinline for instrumentation)


Yes. I will add a new interface to create a SimpleInliner instance for pre-inlining which will ignore the -inline.*-threshold
flags while creating the InlineParams object


================
Comment at: include/llvm/Analysis/InlineCost.h:38
@@ +37,3 @@
+
+// Use when -O[34] is specified.
+const int OptAggressiveThreshold = 275;
----------------
mehdi_amini wrote:
> chandlerc wrote:
> > 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.
> I don't know exactly, all I could find was: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130819/185217.html
> 
> 
I have changed the comment to "Use when -O3 is specified".

================
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.
----------------
chandlerc wrote:
> 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?
I have changed the command line flag's variable name to InlineThreshold (in sync with the "inline-threshold" string)

================
Comment at: include/llvm/Analysis/InlineCost.h:122
@@ +121,3 @@
+  /// Threshold to use for cold callees.
+  Optional<int> ColdThreshold;
+  /// Threshold to use when the caller is optimized for size.
----------------
silvas wrote:
> Can you add a comment about what happens when the "Optional" is in its "empty" state? I assume that `DefaultThreshold` is used in their place but it would be good to document explicitly what happens.
I've expanded the comments. Let me know if you want to add anything else specific. 

================
Comment at: lib/Analysis/InlineCost.cpp:593
@@ -615,1 +592,3 @@
 
+static int Min(int A, Optional<int> B) {
+  return B ? std::min(A, B.getValue()) : A;
----------------
silvas wrote:
> This doesn't match the LLVM naming convention for functions (should start with lower case): http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> 
> Also, you may want to make this 'min' function a local lambda in updateThreshold as it is only used there I think.
Made this into a lambda and renamed it to minIfPresent

================
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 {
----------------
chandlerc wrote:
> I would much prefer these command line flags remain in the InlineCost analysis and next to all the other numbers used to tune things.
I am a bit unsure what you were suggesting but I assume you want to move this to InlineCost.cpp as globals in llvm namespace and reference them in InlineSimple.cpp


https://reviews.llvm.org/D22120