<div dir="ltr"><img class="gmail-ajT" src="https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif"><br><div class="gmail_extra"><br><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="gmail-"><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="gmail-"><br></span></blockquote><div><br></div><div>There are situations where using template does not apply.</div><div><br></div><div>Consider:</div><div><br></div><div>1) </div><div> typedef void (*FP) (void);</div><div> const FP fp[10] = {&foo1, &foo2, ...}; // foo_x marked as always inline</div><div><br></div><div> void bar(int i) {</div><div> fp[i]();</div><div> ...</div><div> }</div><div><br></div><div> void test() {</div><div> bar(0);</div><div> }</div><div><br></div><div>2) </div><div><div> typedef void (*FP) (void);</div><div> const FP fp[10] = {&foo1, &foo2, ...};</div><div><br></div><div> void bar(int i) {</div><div> fp[i]();</div><div> ...</div><div> }</div><div><br></div><div> void test() {</div><div> for (i = 0; i< 2; i++) // fully unrolled before inlining</div><div> bar(i);</div><div> }</div></div><div><br></div><div>3) </div><div><br></div><div> struct B {</div><div> virtual void foo(); //marked as always inline</div><div> };</div><div><br></div><div> struct D: public B {</div><div> void foo() override;</div><div> }</div><div><br></div><div> void bar (B* bp) {</div><div> bp->foo();</div><div> }</div><div><br></div><div> void test() {</div><div> D *dp = new D();</div><div> bar(dp);</div><div> }</div><div><br></div><div>..</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> I think Easwaran's patch is consistent with the current inline behavior -- it boots inlining to callee with indirect callsites that can be turned into direct calls and then further inlined. The amount of boost is the benefit of inlining the indirect callsites (i.e., the difference between inline cost and allowed threshold). In fact, if the CA.analyzeCall call in the existing code<br>
><br>
</span><span class="gmail-">> auto IndirectCallParams = Params;<br>
> IndirectCallParams.<wbr>DefaultThreshold = InlineConstants::<wbr>IndirectCallThreshold;<br>
> CallAnalyzer CA(TTI, GetAssumptionCache, GetBFI, PSI, *F, CS,<br>
</span><span class="gmail-">> IndirectCallParams);<br>
> if (CA.analyzeCall(CS)) {<br>
> ...<br>
><br>
><br>
> is turned into getInlineCost(CS), it will achieve similar effect -- except that InlineCost associated with AlwaysInline is a little too extreme.<br>
><br>
> In many cases, AlwaysInline is very strong hint by the user to indicate performance benefit of inlining the function, it does not make sense to completely ignore it.<br>
<br>
</span>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>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><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
(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>True, and always_inline is used (wrongly) to fit that bill.</div><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>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><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><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. 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><br></div><div>David</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D34471" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34471</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>