<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman <<a href="mailto:eraman@google.com">eraman@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div>First off, thanks for collecting the numbers and broadening the distribution. Also, sorry it took me so long to get over to this thread.</div><div><br></div><div>I want to lay out my stance on this issue at a theoretical and practical level first. I'll follow up with thoughts on the numbers as well after that.</div><div><br></div><div>I think that tying *any* optimizer behavior to the 'inline' keyword is fundamentally the wrong direction. We have reasons why we have done this historically, and we can't just do an immediate about face, but we should be actively looking for ways to *reduce* the optimizer's reliance on this keyword to convey any meaning whatsoever.</div><div><br></div><div>The reason I think that is the correct direction is because, for better or worse, the 'inline' keyword in C++ is not about optimization, but about linkage. It has a functional impact and can be both necessary or impossible to use to meet those functional requirements. This in turn leaves programmers in a lurch if the functional requirements are ever in tension with the optimizer requirements.</div><div><br></div><div>We're also working really hard to get more widely deployed cross-module optimization strategies, in part to free programmers from the requirement that they put all their performance critical code in header files. That makes compilation faster, and has lots of benefits to the factoring and design of the code itself. We shouldn't then create an incentive to keep things in header files so that they pick up a hint to the optimizer.</div><div><br></div><div>Ultimately, the world will be a better place if we can eventually move code away from relying on the hint provided by the 'inline' keyword to the optimizer.</div><div><br></div><div><br></div><div>That doesn't mean that the core concept of hinting to the optimizer that a particular function is a particularly good candidate for inlining is without value. While I think it is a bad practice that we shouldn't encourage in code (especially portable code) I can see the desire to at least have *some* attribute which is nothing more or less than a hint to the optimizer to inline harder[1]. It would help people work around inliner bugs in the short term, and even help debug inliner-rooted optimization problems. Codebases with strong portability requirements could still (and probably should) forbid or tightly control access to this kind of hint. I would want really strong documentation about how this attribute *completely voids* your performance warranty (if such a thing exists) as from version to version of the compiler it may go from a helpful hint to a devastatingly bad hint. But I think I could be persuaded to live with such a hint existing. But I'm *really* uncomfortable with it being tied to something that also impacts linkage or other semantics of the program.</div><div> </div><div>[1]: Currently, the only other hint we have available is pretty terrible as it *also* has semantic effects: the always_inline attribute.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The proposed patch will add InlineHint to methods defined inside a class:<br>
<br>
--- a/lib/CodeGen/CodeGenFunction.cpp<br>
+++ b/lib/CodeGen/CodeGenFunction.cpp<br>
@@ -630,7 +630,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,<br>
if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {<br>
if (!CGM.getCodeGenOpts().NoInline) {<br>
for (auto RI : FD->redecls())<br>
- if (RI->isInlineSpecified()) {<br>
+ if (RI->isInlined()) {<br>
Fn->addFnAttr(llvm::Attribute::InlineHint);<br>
break;<br>
}<br>
<br>
Here are the performance and size numbers I've collected:<br>
<br>
<br>
- C++ subset of Spec: No performance effects, < 0.1% size increase<br>
(all size numbers are text sizes returned by 'size')<br>
- Clang: 0.9% performance improvement (both -O0 and -O2 on a large .ii<br>
file) , 4.1% size increase<br></blockquote><div><br></div><div>FWIW, this size increase seems *really* bad. I think that kills this approach even in the short term.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Chrome: no performance improvement, 0.24% size increase<br>
- Google internal benchmark suite (geomean of ~20 benchmarks): ~1.8%<br>
performance improvement, no size regression<br></blockquote><div><br></div><div>I'm also somewhat worried about the lack of any performance improvements outside of the Google benchmarks. That somewhat strongly suggests that our benchmarks are overly coupled to this hint already. The fact that neither Chrome, Clang, nor SPEC improved is... not at all encouraging.</div><div><br></div><div>-Chandler</div></div></div>