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

[Xinliang David Li via llvm-commits]

On Fri, Jul 29, 2016 at 10:29 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Fri, Jul 29, 2016 at 10:13 PM Xinliang David Li via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> 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.
>
>
> This was discussed among various parties when the behavior of O4 was
> changed. I don't recall where, but I don't think this patch is the place to
> re-debate this issue.

>
>>
>> >
>> > 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.
>
>
> Ah, yes. I think the command line flag's variable name should change as when
> specified it is in fact not a default but it overrides the default. =]
> (I completely agree about not having the same name for both.)


right.

thanks,

David