[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
https://reviews.llvm.org/D22120
More information about the llvm-commits
mailing list