<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 7, 2015 at 10:25 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman <<a href="mailto:eraman@google.com">eraman@google.com</a>> 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 thread.<br>
><br>
> I want to lay out my stance on this issue at a theoretical and practical<br>
> level first. I'll follow up with thoughts on the numbers as well after that.<br>
><br>
> I think that tying *any* optimizer behavior to the 'inline' keyword is<br>
> fundamentally the wrong direction.<br>
<br>
</span>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>
<span class=""><br>
>We have reasons why we have done this<br>
> historically, and we can't just do an immediate about face, but we should be<br>
> actively looking for ways to *reduce* the optimizer's reliance on this<br>
> keyword to convey any meaning whatsoever.<br>
<br>
</span>yes those additional things will be done, but they are really orthogonal.<br>
<span class=""><br>
><br>
> The reason I think that is the correct direction is because, for better or<br>
> worse, the 'inline' keyword in C++ is not about optimization, but about<br>
> linkage.<br>
<br>
</span>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>
"A function declaration (8.3.5, 9.3, 11.3) with an inline specifier<br>
declares an inline function. The inline specifier indicates to the<br>
implementation that inline substitution of the function body at the<br>
point of call is to be preferred to the usual function call mechanism.<br>
An implementation is not required to perform this inline substitution<br>
at the point of call; however, even if this inline substitution is<br>
omitted, the other rules for inline functions defined by 7.1.2 shall<br>
still be respected."<br></blockquote><div><br></div><div>This describes Clang's current behavior, not the behavior you propose.</div><div><br></div><div>Note that there is a difference between an "inline specifier" (meaning that the inline keyword appeared in the declaration, see the grammar description in paragraph 1 of 7.1.2, and the introductory text in 7.1) and a function being an "inline function" (meaning that multiple definitions are permitted in different translation units, see 3.2/4 and /6).</div><div><br></div><div>The above wording clearly says that the inline *specifier* is a hint that we should inline the function, and...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Developers see those and rely on those to give compiler the hints.<br>
<br>
Most importantly, paragraph 3 says:<br>
<br>
"A function defined within a class definition is an inline function.<br></blockquote><div><br></div><div>... the same is *not* true for a function definition that appears within a class definition. That is merely an inline function (that is, it can be defined in multiple translation units).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The inline specifier shall not appear on a block scope function<br>
declaration.93 If the inline specifier is used in a friend<br>
declaration, that declaration shall be a definition or the function<br>
shall have previously been declared inline."<br>
<br>
Here we can see regardless of how optimizer will honor the hint and to<br>
what extent, and based on what analysis,<br>
it is basically incorrect to drop the attribute on the floor for<br>
in-class function definitions. Eswaran's fix is justified with this<br>
reason alone. The side effect of changing inliner behavior is<br>
irrelevant.<br>
<span class=""><br>
> It has a functional impact and can be both necessary or impossible<br>
> to use to meet those functional requirements. This in turn leaves<br>
> programmers in a lurch if the functional requirements are ever in tension<br>
> with the optimizer requirements.<br>
<br>
</span>Not sure what you mean. Performance conscious programmers use it all the time.<br>
<span class=""><br>
><br>
> We're also working really hard to get more widely deployed cross-module<br>
> optimization strategies, in part to free programmers from the requirement<br>
> that they put all their performance critical code in header files. That<br>
> makes compilation faster, and has lots of benefits to the factoring and<br>
> design of the code itself. We shouldn't then create an incentive to keep<br>
> things in header files so that they pick up a hint to the optimizer.<br>
<br>
><br>
> Ultimately, the world will be a better place if we can eventually move code<br>
> away from relying on the hint provided by the 'inline' keyword to the<br>
> optimizer.<br>
><br>
<br>
</span>While I would like to see that happen some day, I do think it is an<br>
independent matter.<br>
<span class=""><br>
><br>
> That doesn't mean that the core concept of hinting to the optimizer that a<br>
> particular function is a particularly good candidate for inlining is without<br>
> value.<br>
<br>
</span>yes.<br>
<span class=""><br>
>While I think it is a bad practice that we shouldn't encourage in<br>
> code (especially portable code)<br>
<br>
</span>yes -- there are indeed programmers who use this casually without<br>
considering performance.<br>
<span class=""><br>
> I can see the desire to at least have *some*<br>
> attribute which is nothing more or less than a hint to the optimizer to<br>
> inline harder[1].<br>
<br>
</span>yes -- there are programmers who use the attribute consciously.<br>
<span class=""><br>
> It would help people work around inliner bugs in the short<br>
> term, and even help debug inliner-rooted optimization problems.<br>
<br>
</span>I think it is a good hint to the compiler even in the longer term.<br>
With PGO, we should minimize the reliance on the hint though.<br>
<span class=""><br>
>Codebases<br>
> with strong portability requirements could still (and probably should)<br>
> forbid or tightly control access to this kind of hint. I would want really<br>
> strong documentation about how this attribute *completely voids* your<br>
> performance warranty (if such a thing exists) as from version to version of<br>
> the compiler it may go from a helpful hint to a devastatingly bad hint.<br>
<br>
</span>Why? If the compiler becomes smarter and smarter, the inline hint will<br>
become more and more irrelevant and eventually has no effect -- why<br>
would the performance warranty be voided? If the compiler is not yet<br>
smart enough, why would the compiler refuse to take the hint and<br>
forbid developer provide the hint?<br>
<span class=""><br>
> But<br>
> I think I could be persuaded to live with such a hint existing. But I'm<br>
> *really* uncomfortable with it being tied to something that also impacts<br>
> linkage or other semantics of the program.<br>
<br>
</span>For consistent with standard, we should pass the attribute. Linkage is<br>
not affected in anyway.<br>
<span class=""><br>
><br>
> [1]: Currently, the only other hint we have available is pretty terrible as<br>
> it *also* has semantic effects: the always_inline attribute.<br>
><br>
><br>
>><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>
><br>
><br>
> FWIW, this size increase seems *really* bad. I think that kills this<br>
> approach even in the short term.<br>
<br>
</span>Re. size and performance trade-off -- 0.9% performance improvement<br>
should greatly win the size cost. Besides among all programs see, only<br>
clang sees this size increase with all the others seeing negligible<br>
size increase.<br>
<br>
This is not a short term vs long term situation. It is basically a bug<br>
fix that FE drops the attribute. If it exposes inliner heuristic bug,<br>
that should be fixed/tuned separately. With the hint correctly passed<br>
in, Easwaran will do further tuning including time based analysis.<br>
<span class=""><br>
><br>
>><br>
>> - 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>
><br>
><br>
> I'm also somewhat worried about the lack of any performance improvements<br>
> outside of the Google benchmarks. That somewhat strongly suggests that our<br>
> benchmarks are overly coupled to this hint already. The fact that neither<br>
> Chrome, Clang, nor SPEC improved is... not at all encouraging.<br>
<br>
</span>Other than Google benchmarks, we do see Clang improve performance.<br>
Besides, current inliner needs to be further tuned in order to get<br>
more performance benefit. Passing the hint through is simply an<br>
enabler. Also remember that most of SPEC benchmarks are C programs.<br>
C++ programs with heavy use of virtual functions may not benefit a lot<br>
either.<br>
<br>
David<br>
<br>
<br>
><br>
> -Chandler<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>