<div dir="ltr">Just to close the loop on one of the open questions here (seems we have a reasonable path forward generally):<br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 23, 2017 at 3:51 PM Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br></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_extra"><div class="gmail_quote">On Fri, Jun 23, 2017 at 12:41 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:</div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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>2) weaknesses in the design of the code (making it impossible to get reasonable performance without heroics of inlining or impossible for a profile to produce the necessary inlining hints)</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I don't see how this prevents profile to produce necessary inlining hints -- but I do see this part missing (using profile to estimate benefit of indirect call inlining) from the current inliner.</div></div></div></div></blockquote><div><br></div><div>I've seen code that essentially forces (unnecessarily) very hot and very cold to code paths that are different to share a large region of code, so that it is hard to get the profile information needed into the right place. Essentially, a very deep call stack which is almost entirely the same between hot path and cold path so that the call site which is actually "hot" and distinguishes all of this is very far away from the thing we need to optimize.</div><div><br></div><div>But this seems really rare and maybe not important / a distraction. Sorry for that. I've seen it only once in a parser where a pile of parsing logic was shared between hot and cold paths and it made it hard to get a hot-path optimized version because *so much* code had to be specialized. It ended up being hard to manage the code in that form as well (different parsing approaches were desired in the hot path) so it made sense to fix the source code design eventually.</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_extra"><div class="gmail_quote"><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>3) an inability to get a profile or use static hints</div><div><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>by' ... to get a profile', you mean the inability of the user to use PGO?</div></div></div></div></blockquote><div><br></div><div>Yeah.</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_extra"><div class="gmail_quote"><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></div><div>Or something along these lines. Essentially, it's an escape hatch when other things that *should* allow the inliner to work aren't enough. I absolutely think it is a necessary thing to have, but I think this should guide how it works.</div><div><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The static hints available today are indeed not enough. For instance 'hot' attribute applies to the function, not the callsite. builtin_expect applies to a branch locally, and is not enough to indicate the importance of a callsite either.</div></div></div></div></blockquote><div><br></div><div>Completely agree. Very happy to see more work designing a more expressive and useful set of source annotations. Especially ones that carry clear enough semantics that programmers both can use them when needed and also *don't* use them when *bad*. Historically, the latter has been an even bigger problem than the former. </div><div><br></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_extra"><div class="gmail_quote"><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></div><div>For example, I think it is actually reasonable for this attribute to be very localized and *not* impact the thresholds w.r.t. indirect calls in other functions. The reason is that it seems like it should both be as simple of a heuristic adjustment and localized as possible.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>With the current inline design, a function level inline related attribute will need affect thresholds of all callsites, thus won't be 'localized' in order to be effective, or do you mean something else?</div></div></div></div></blockquote><div><br></div><div>This gets back to the point you make which I totally agree with -- eventually we really need the 'strong inline hint' annotation to be available *both* on a function but also on a *call*.</div><div><br></div><div>While we could productively start with a quick version that was a function-level attribute, I think we would want to as an immediate next step (and pretty clear plan at the beginning) extend it to calls so that it can be effective in cases where a function level annotation is too broad.</div><div><br></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_extra"><div class="gmail_quote"><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 also think this is important for *users* of a library to be aware of -- these use cases don't make a lot of sense as some fundamental part of the abstractions of a library that are hidden from the users of the library.</div><div><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This hidden information does not affect library interfaces but just the underlying implementation, so the users probably don't care. </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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><div><br></div><div>All of this said, it is possible that we'll encounter real-world use cases for a strong attribute hint that require it to have a complex interaction with inlining heuristics like what it in this patch. But as the only real-world case I know of is easily addressed with a template, I'd prefer to wait to have this complexity in the inline cost heuristics based on some case where we really can't solve or work around the problem in any other way. The inliner's heuristics are already complex, and so it seems good to avoid adding more here.</div><span><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_extra"><div class="gmail_quote"><div>We should implement this performance heuristic for always inline indirect calls now, and when the new attribute is available, emit warnings about the wrong use of always inline and deprecate this eventually. </div></div></div></div></blockquote><div><br></div></span><div>Generally, unless this is going to take some very large body of technical work, it seems like we should just start off with the correct long term design. For example, just adding a function definition attribute that conveys the desired meaning should be quite easy. The design and implementation of call-site annotations can be done separately and wouldn't block this. But hopefully all of this is moot given that there are source workarounds for the practical cases that I know of and we might be able to take a different approach for any future issues that arise.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>ok, that sounds fine.</div><div><br></div><div>David</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"><span class="m_-9000926022021823808HOEnZb"><font color="#888888"><div><br></div><div>-Chandler</div></font></span></div></div>
</blockquote></div><br></div></div>
</blockquote></div></div>