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

[Xinliang David Li via llvm-commits]

On Fri, Jul 29, 2016 at 9:39 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> 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.

Compilation model is implementation detail. It is perfectly fine to
implement certain optimization level using different compilation model
if produces optimization that matches user's expectation of that
level.    You are making implicit assumption that (user expects)
different optimization uses the same compilation model that does not
seem to be true. In fact, user familiar with other compilers may
actually expect the opposite.

>
> 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?

-inline-threshold option is named 'DefaultInlineThreshold'.   It
collides with DefaultThreshold member variable in simple inliner, but
means differently. What I suggested is one of them needs to change
name.

David

>
> ================
> 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
>
>
>