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

[Chandler Carruth via llvm-commits]

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.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160730/4fdc0f7f/attachment.html>