<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 8, 2015 at 10:40 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Wed, Jul 8, 2015 at 1:46 PM Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">----- Original Message -----<br>
> From: "Xinliang David Li" <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
> To: "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>><br>
> Cc: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>, "<<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>> List" <<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>><br>
> Sent: Wednesday, July 8, 2015 12:25:18 AM<br>
> Subject: Re: [LLVMdev] Inline hint for methods defined in-class<br>
><br>
> On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth<br>
> <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br>
> > On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman <<a href="mailto:eraman@google.com" target="_blank">eraman@google.com</a>><br>
> > wrote:<br>
> >><br>
> >> I'm reviving this thread after a while and CCing cfe-commits as<br>
> >> suggested by David Blaikie. I've also collected numbers building<br>
> >> chrome (from chromium, on Linux) with and without this patch as<br>
> >> suggested by David. I've re-posted the proposed patch and<br>
> >> performance/size numbers collected at the top to make it easily<br>
> >> readable for those reading it through cfe-commits.<br>
> ><br>
> ><br>
> > First off, thanks for collecting the numbers and broadening the<br>
> > distribution. Also, sorry it took me so long to get over to this<br>
> > thread.<br>
> ><br>
> > I want to lay out my stance on this issue at a theoretical and<br>
> > practical<br>
> > level first. I'll follow up with thoughts on the numbers as well<br>
> > after that.<br>
> ><br>
> > I think that tying *any* optimizer behavior to the 'inline' keyword<br>
> > is<br>
> > fundamentally the wrong direction.<br>
><br>
> Chandler, thanks for sharing your thought -- however I don't think it<br>
> is wrong, let alone 'fundamentally wrong'.  Despite all the analysis<br>
> that can be done, the inliner is in the end heuristic based. In lack<br>
> of the profile data, when inlining two calls yield the same static<br>
> benefit and size cost, it is reasonable for the inliner to think the<br>
> call to the function with inline hint to yield more high<br>
> dynamic/runtime benefit -- thus it has a higher static size budget to<br>
> burn.<br>
><br>
> >We have reasons why we have done this<br>
> > historically, and we can't just do an immediate about face, but we<br>
> > should be<br>
> > actively looking for ways to *reduce* the optimizer's reliance on<br>
> > this<br>
> > keyword to convey any meaning whatsoever.<br>
><br>
> yes those additional things will be done, but they are really<br>
> orthogonal.<br>
><br>
> ><br>
> > The reason I think that is the correct direction is because, for<br>
> > better or<br>
> > worse, the 'inline' keyword in C++ is not about optimization, but<br>
> > about<br>
> > linkage.<br>
><br>
> It is about both optimization and linkage. In fact the linkage simply<br>
> serves as an implementation detail. In C++ standard 7.1.2,  paragraph<br>
> 2 says:<br>
<br>
The fact that C++ combines, into one keyword, a change in semantics (linkage) and an optimization hint is quite unfortunate. I wish it were otherwise.</blockquote><div><br></div></div></div><div>We could work to change it? I specifically proposed adding a way to move away from this unfortunate design.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> However, as it stands, I support this change. The benchmark numbers are encouraging, and it replaces an implementation quirk with the underlying (unfortunate) language design choice. The implementation quirk is that putting the inline keyword on an in-class function definition changes the behavior of the optimizer. However, according to the language specification, that definition should have implied that keyword. While an implementation is certainly free to do arbitrary things with hints, this behavior violates the spirit of the language specification.</blockquote><div><br></div></span><div>I strongly disagree that this is the spirit of the language specification. Even if it was historically, I think we should move away from that. The language shouldn't be trying to do this with a language keyword, and it shouldn't be coupling semantics to hints. I'm very happy to take this up with the committee, but I don't see why we shouldn't push Clang in that direction here when there is no issue of conformance.</div><div><br></div><div>To see how broken this is, let's look at how miserably small the difference is between the un-hinted and hinted thresholds. We've ended up shrinking this difference over time in LLVM because increasing the hinted threshold caused lots of performance regressions and size regressions.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It makes a meaningless use of a standardized keyword meaningful, and that's the greater transgression.</blockquote><div><br></div></span><div>So here is what I want to do:</div><div><br></div><div>1) Add a non-semantic attribute that conveys this hint. We could even convey a much *stronger* hint with this rather than just a tiny hint the way it is today because it wouldn't end up being forced onto every template regardless of whether that makes sense.</div><div><br></div><div>2) Start lobbying to remove the hint from the 'inline' keyword by working with the people who see regressions from this to use the new annotation to recover the performance.</div><div><br></div><div>3) Completely remove the semantic coupling of the optimizer hint and fix the meaningless use of the standardized keyword at the same time.</div><div><br></div><div>But the more places where we use the inline hint today, the harder #2 will become. I've already tried once before to remove the hint and couldn't because of benchmarks that had been tightly tuned and coupled to the existing (quirky) behavior. I really think that doing this more will make getting to #3 harder. Making progress toward a cleaner design harder seems worse than coping with the quirks that have existed in Clang for over 5 years for another few years.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In addition, it does tend to be the case that in-class function definitions are small and suitable for inlining.<br></blockquote><div><br></div></span><div>But if they are small and suitable for inlining, won't the existing threshold work just fine?</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I agree. A 1% performance increase is worth a 4% code-size increase when not optimizing for size.<br></blockquote><div><br></div></span><div>I really don't. Maybe in -O3 or something, but a 4% code-size increase is a hard regression to swallow. Especially in a single benchmark, and where many other benchmarks show no benefit at all. This isn't a matter of "most code gets better, so we need to tolerate the unfortunate variance of size".</div></div></div></blockquote><div><br></div><div><br></div><div>Suppose we take out the specially treatment of 'inline' hint completely in inliner which results in 1% loss in clang and size improvement of 4%, would you be ok to take that change?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>I know its not really "fair" to view regressions as more important than missed opportunities, but the reality is that regressions *are* more problematic than missed opportunities.</div></div></div>
<br></blockquote><div><br></div><div>You need to be more specific on why the regression is problematic (and more so than missed opportunities).  Text size increase (shared and much smaller than heap) is usually not a big issue except for tiny devices which is likely to use Os/Oz in the build. For servers, there are are simple rules to compare cpu vs ram resource cost.</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" rel="noreferrer" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div></div>