On Tue, Jul 7, 2015 at 10:25 PM, Xinliang David Li <davi...@google.com> wrote:
> On Tue, Jul 7, 2015 at 6:06 PM, Chandler Carruth <chandl...@gmail.com> > wrote: > > On Tue, Jul 7, 2015 at 4:11 PM Easwaran Raman <era...@google.com> wrote: > >> > >> I'm reviving this thread after a while and CCing cfe-commits as > >> suggested by David Blaikie. I've also collected numbers building > >> chrome (from chromium, on Linux) with and without this patch as > >> suggested by David. I've re-posted the proposed patch and > >> performance/size numbers collected at the top to make it easily > >> readable for those reading it through cfe-commits. > > > > > > First off, thanks for collecting the numbers and broadening the > > distribution. Also, sorry it took me so long to get over to this thread. > > > > 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. > > > > I think that tying *any* optimizer behavior to the 'inline' keyword is > > fundamentally the wrong direction. > > Chandler, thanks for sharing your thought -- however I don't think it > is wrong, let alone 'fundamentally wrong'. Despite all the analysis > that can be done, the inliner is in the end heuristic based. In lack > of the profile data, when inlining two calls yield the same static > benefit and size cost, it is reasonable for the inliner to think the > call to the function with inline hint to yield more high > dynamic/runtime benefit -- thus it has a higher static size budget to > burn. > > >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. > > yes those additional things will be done, but they are really orthogonal. > > > > > 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 is about both optimization and linkage. In fact the linkage simply > serves as an implementation detail. In C++ standard 7.1.2, paragraph > 2 says: > > "A function declaration (8.3.5, 9.3, 11.3) with an inline specifier > declares an inline function. The inline specifier indicates to the > implementation that inline substitution of the function body at the > point of call is to be preferred to the usual function call mechanism. > An implementation is not required to perform this inline substitution > at the point of call; however, even if this inline substitution is > omitted, the other rules for inline functions defined by 7.1.2 shall > still be respected." > This describes Clang's current behavior, not the behavior you propose. 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). The above wording clearly says that the inline *specifier* is a hint that we should inline the function, and... > Developers see those and rely on those to give compiler the hints. > > Most importantly, paragraph 3 says: > > "A function defined within a class definition is an inline function. > ... 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). > The inline specifier shall not appear on a block scope function > declaration.93 If the inline specifier is used in a friend > declaration, that declaration shall be a definition or the function > shall have previously been declared inline." > > Here we can see regardless of how optimizer will honor the hint and to > what extent, and based on what analysis, > it is basically incorrect to drop the attribute on the floor for > in-class function definitions. Eswaran's fix is justified with this > reason alone. The side effect of changing inliner behavior is > irrelevant. > > > 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. > > Not sure what you mean. Performance conscious programmers use it all the > time. > > > > > 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. > > > > > 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. > > > > While I would like to see that happen some day, I do think it is an > independent matter. > > > > > 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. > > yes. > > >While I think it is a bad practice that we shouldn't encourage in > > code (especially portable code) > > yes -- there are indeed programmers who use this casually without > considering performance. > > > 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]. > > yes -- there are programmers who use the attribute consciously. > > > It would help people work around inliner bugs in the short > > term, and even help debug inliner-rooted optimization problems. > > I think it is a good hint to the compiler even in the longer term. > With PGO, we should minimize the reliance on the hint though. > > >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. > > Why? If the compiler becomes smarter and smarter, the inline hint will > become more and more irrelevant and eventually has no effect -- why > would the performance warranty be voided? If the compiler is not yet > smart enough, why would the compiler refuse to take the hint and > forbid developer provide the 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. > > For consistent with standard, we should pass the attribute. Linkage is > not affected in anyway. > > > > > [1]: Currently, the only other hint we have available is pretty terrible > as > > it *also* has semantic effects: the always_inline attribute. > > > > > >> > >> The proposed patch will add InlineHint to methods defined inside a > class: > >> > >> --- a/lib/CodeGen/CodeGenFunction.cpp > >> +++ b/lib/CodeGen/CodeGenFunction.cpp > >> @@ -630,7 +630,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, > >> if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) { > >> if (!CGM.getCodeGenOpts().NoInline) { > >> for (auto RI : FD->redecls()) > >> - if (RI->isInlineSpecified()) { > >> + if (RI->isInlined()) { > >> Fn->addFnAttr(llvm::Attribute::InlineHint); > >> break; > >> } > >> > >> Here are the performance and size numbers I've collected: > >> > >> > >> - C++ subset of Spec: No performance effects, < 0.1% size increase > >> (all size numbers are text sizes returned by 'size') > >> - Clang: 0.9% performance improvement (both -O0 and -O2 on a large .ii > >> file) , 4.1% size increase > > > > > > FWIW, this size increase seems *really* bad. I think that kills this > > approach even in the short term. > > Re. size and performance trade-off -- 0.9% performance improvement > should greatly win the size cost. Besides among all programs see, only > clang sees this size increase with all the others seeing negligible > size increase. > > This is not a short term vs long term situation. It is basically a bug > fix that FE drops the attribute. If it exposes inliner heuristic bug, > that should be fixed/tuned separately. With the hint correctly passed > in, Easwaran will do further tuning including time based analysis. > > > > >> > >> - Chrome: no performance improvement, 0.24% size increase > >> - Google internal benchmark suite (geomean of ~20 benchmarks): ~1.8% > >> performance improvement, no size regression > > > > > > 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. > > Other than Google benchmarks, we do see Clang improve performance. > Besides, current inliner needs to be further tuned in order to get > more performance benefit. Passing the hint through is simply an > enabler. Also remember that most of SPEC benchmarks are C programs. > C++ programs with heavy use of virtual functions may not benefit a lot > either. > > David > > > > > > -Chandler > _______________________________________________ > cfe-commits mailing list > cfe-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits