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

Easwaran Raman via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 1 14:49:52 PDT 2016

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


More information about the llvm-commits mailing list