<div dir="ltr"><br><div class="gmail_extra"><br><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:<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"><span class=""><div dir="ltr">On Thu, Jun 22, 2017 at 10:43 PM Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</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"><img class="m_38609867370937383m_7007958627434291260gmail-ajT" src="https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif"><br><div class="gmail_extra"><br><div class="gmail_quote"></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 22, 2017 at 9:14 PM, Chandler Carruth via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">chandlerc added a comment.<br>
<span class="m_38609867370937383m_7007958627434291260gmail-"><br>
In <a href="https://reviews.llvm.org/D34471#788736" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34471#788736</a>, @davidxl wrote:<br>
<br>
> Chandler's example does not involve any indirect calls so it is not related to this patch.<br>
<br>
<br>
</span>Actually, it is...<br>
<br>
The original test case doesn't look exactly like my example. Rather than a template argument and a call through that template argument, it uses an indirect call to a normal function argument.<br>
<br>
My suggestion is that the code should be changed to use a template argument if the intent is to make `always_inline` actually *always* inline.<br>
<span class="m_38609867370937383m_7007958627434291260gmail-"><br></span></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>There are situations where using template does not apply.</div></div></div></div></blockquote><div><br></div></span><div>I understand that there are hypothetical situations, but the only concrete use case I'm aware of isn't one of them... If there are other benchmarks where this matters, I'd be interested in which ones.</div><div><br></div><div>However, the fact that there might be other interesting cases is why I brought up an explicit attribute, I just don't think we should handle it in this way. See my more detailed response below, as I think that's the more interesting discussion.</div><span class=""><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"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I don't think this is really the right way to model the `always_inline` attribute. I continue to think that this attribute should mean that we very literally *always* inline. See the discussion here for more details:<br>
<a href="http://lists.llvm.org/pipermail/llvm-dev/2015-August/089466.html" rel="noreferrer" target="_blank">http://lists.llvm.org/<wbr>pipermail/llvm-dev/2015-<wbr>August/089466.html</a></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I don't disagree with this. In reality, always_inline is not used in that sense literally -- solving that problem (fixing user sources) is a different problem, especially when the alternative way to suggest a very strong inline hint does not yet exist.</div></div></div></div></blockquote><div><br></div></span><div>But we could easily add it if it is in fact needed. I would rather do that than add more logic around always inline.</div></div></div></blockquote><div><br></div><div>Adding this is certainly easy, but the adoption of the new attributes may be an issue due to portability concerns.</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>Let's assume that we *do* actually add a strong inline hint, and the rest of this discussion is about that attribute, because I agree that is a much more interesting scenario, and you're raising important points for that scenario...</div><span class=""><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"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">(Note that I *also* am still in favor of having an attribute that is actually a very strong hint to do inlining, I just don't think any of the existing attributes really fit that bill,</blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>True, and always_inline is used (wrongly) to fit that bill.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> and I don't think the original motivating test case is a particularly compelling example. Even if a user *does* want to get really strong inlining because of a weird indirect function call, I don't know why the inliner has to go to great lengths to detect this rather than expecting the user to also attribute the function receiving the function pointer...)<br>
<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Firstly, The user/(indirect) caller of the function (marked as always inline) and the function itself are likely written by different authors. The library function provider may mark the function with the attribute, and the client/user may not even aware of the attribute (and the benefit of inlining it) -- only the compiler/inliner knows about it, so it is not the job of the client code to mark the caller to be always inline.<br></div></div></div></div></blockquote><div><br></div></span><div>While this is true, I tend to agree with Eli that this still has a really unacceptable risk of rapid and unexpected code growth.</div><div><br></div><div>Also, I think that at a somewhat fundamental level this misses the point of this kind of attribute. See below, as the purpose of having this kind of strong hint I think is relevant to several of these issues.</div><span class=""><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><div><br></div><div>Secondly, marking the caller (of an indirect callee with always_inline attribute) always_inline is usually not the right way to go. The right way is to mark the callsite as always inline.</div></div></div></div></blockquote><div><br></div></span><div>I absolutely agree that at least in some cases this needs to be applied to call sites. That's one advantage of this being a new attribute, we can work to design it in a way that supports this kind of use case.</div><div><br></div><div>I still think there are reasons to want this on a function definition in some cases, but definitely not all.</div><span class=""><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>Thirdly, the motivation of this patch is not to 'implement' inlining of indirect calls to always inline functions, but as an inline heuristics for better performance. I guess there will be no objection to the patch if the attribute checked is not 'always_inline' but the new very strong inline hint Chandler suggested. In other words, this is something inliner needs to do one way or the other.</div></div></div></div></blockquote><div><br></div></span><div>So, I think a key thing here is what the purpose of these kinds of attributes are...</div><div><br></div><div>I primarily view the utility of a strong inline hint as a way to work around some combination of</div><div>1) weaknesses in the inliner itself</div></div></div></blockquote><div><br></div><div>Agreed. In fact a lot of use cases of always-inline attribute (for performance reasons) can be used as test cases for improving inline analysis.</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>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>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><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>by' ... to get a profile', you mean the inability of the user to use PGO?</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></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>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><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>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><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>This hidden information does not affect library interfaces but just the underlying implementation, so the users probably don't care. </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><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 class=""><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>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="HOEnZb"><font color="#888888"><div><br></div><div>-Chandler</div></font></span></div></div>
</blockquote></div><br></div></div>