<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 10:13 PM Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Jul 29, 2016 at 9:39 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br>
> chandlerc added inline comments.<br>
><br>
> ================<br>
> Comment at: include/llvm/Analysis/InlineCost.h:38<br>
> @@ +37,3 @@<br>
> +<br>
> +// Use when -O[34] is specified.<br>
> +const int OptAggressiveThreshold = 275;<br>
> ----------------<br>
> davidxl wrote:<br>
>> mehdi_amini wrote:<br>
>> > davidxl wrote:<br>
>> > > mehdi_amini wrote:<br>
>> > > > There is no O4, did you mean `O{1,2,3}` here?<br>
>> > > Clang driver maps O4 to O3 (so O4 is actually 'supported').<br>
>> > 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).<br>
>> > That said, I don't really mind if you want to add O4, but clarify the situation for O1/O2.<br>
>> ok -- this should be cleaned up then (it was in the original code that got moved here).<br>
>><br>
>> By the way, why deprecating O4?  It is common to use it  to turn on LTO.<br>
> Because LTO requires a complete change to the compilation model -- it isn't just an optimization level.<br>
<br>
Compilation model is implementation detail. It is perfectly fine to<br>
implement certain optimization level using different compilation model<br>
if produces optimization that matches user's expectation of that<br>
level.    You are making implicit assumption that (user expects)<br>
different optimization uses the same compilation model that does not<br>
seem to be true. In fact, user familiar with other compilers may<br>
actually expect the opposite.<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> 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.<br>
><br>
> 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.<br>
<br>
<br>
><br>
> ================<br>
> Comment at: include/llvm/Analysis/InlineCost.h:118<br>
> @@ +117,3 @@<br>
> +  /// The default threshold to start with for a callee.<br>
> +  int DefaultThreshold;<br>
> +  /// Threshold to use for callees with inline hint.<br>
> ----------------<br>
> davidxl wrote:<br>
>> davidxl wrote:<br>
>> > I prefer this one be called 'InlineThreshold' (to not be confused with the internal option) and document how it is set.<br>
>> Or perhaps keeping this parameter name but changing the option name to be InlineThreshold (not Default...)<br>
> I like calling it "default" if it is OptSize is and such are going to override it.<br>
><br>
> Which parameter were you thinking that is currently named default?<br>
<br>
-inline-threshold option is named 'DefaultInlineThreshold'.   It<br>
collides with DefaultThreshold member variable in simple inliner, but<br>
means differently. What I suggested is one of them needs to change<br>
name.<br></blockquote><div> </div><div>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. =]</div><div>(I completely agree about not having the same name for both.)</div></div></div>