<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 14, 2017 at 6:44 PM, 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span class="gmail-"><div dir="ltr">On Mon, Aug 14, 2017 at 6:38 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 14, 2017 at 6:31 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><br>
In <a href="https://reviews.llvm.org/D36726#841627" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36726#841627</a>, @davidxl wrote:<br>
<br>
> I will provide more comments on propagating the inline hint later. However, I do think it is wrong to propagate the cold attribute in inliner -- there should be already an inter-procedural attribute propagation pass that does this.<br>
<br>
<br>
</span>While I'd love to teach our IPO attribute propagation to do that, we might still need to do it here. The inliner might make the opportunity for this propagation visible and then delete the call removing the chance to do it all within a single pass run, and the interprocedural pass never get a chance to see the intermediate state.<br></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Not sure I understand this. Do you have an example showing that IPO prop won't work?</div></div></div></div></blockquote><div><br></div></span><div>The inliner initially sees a direct call from A to B. The returned value is a pointer which is in turn called. Neither A nor B are cold. Be returns the address of a function C which is cold.</div><div><br></div><div>After inlining, A has a direct call to C (a cold function). The inliner can iterate to this without any other pass running and inline C into A.</div></div></div></blockquote><div><br></div><div><br></div><div>I doubt this most simplistic scenario is common. Besides, Most likely, the inlining to the cold callsite won't actually happen -- after we discover that A is also cold, why would we want to inline a cold callsite into a cold caller? Or unless the implementation does not require the inline transformation to happen to propagate the cold attribute.</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"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>There is no longer a call edge to a cold function, we've lost the chance to propagate anything. =/</div></div></div></blockquote><div><br></div><div>IMO, a powerful IPA should not depend too much on inlining, but should instead more integrated with inter-procedural constant/value propagation(analysis only). There are drawbacks of relying on inlining to do the propagation:</div><div><br></div><div>1) The (exposed) cold callsite is not inlined into the caller; </div><div>2) the cold attribute gets discovered after some damage has been done. With your example:</div><div><br></div><div> a() {</div><div> ptr = b();</div><div> ptr();</div><div> }</div><div><br></div><div> b() {</div><div> return &cold_c;</div><div> }</div><div><br></div><div> When cold_c is exposed to a(), we found that a() is cold too, but b() is already inlined into a() resulting in unnecessary code bloat. If the IPA propagation discovers ptr's set include only cold callees, a() is marked as cold, and b won't be inlined into a later.</div><div><br></div><div>Slightly more complicated cases will be missed:</div><div><br></div><div><div> a() {</div><div> ptr = b(s);</div><div> ptr();</div><div> }</div><div><br></div><div> b(int s) {</div><div> if (s) </div><div> return &cold_c;</div><div> else </div><div> return &cold_d;</div><div> }</div></div><div><br></div><div>or it has a deep call chain to get the function pointer, or for some reason 'b' can not be inlined.</div><div><br></div><div><br></div><div>3) Another example. Suppose a() is a hot function, b() is a cold function discovered via inlining and cold attribute propagation:</div><div><br></div><div> a() {</div><div> if (...) {</div><div> b(&cold_c); // cold site</div><div> }</div><div> }</div><div><br></div><div> b( void(*ptr)() ) {</div><div> ptr();</div><div> }</div><div><br></div><div> </div><div>Here, we can add more code to teach inliner to discover the callsite to 'b' is cold, but it will fail with slightly more complicated example where bottom up propagation of values is not enough (tied with the inlining):</div><div><br></div><div> x() {</div><div> ...</div><div> a(&cold_c); // we see all callsites to 'a' with cold function ptr passed in</div><div> ..</div><div> }</div><div><br></div><div> a(void (*ptr) () ) { // hot </div><div> </div><div> if (...) {</div><div> b(ptr); // cold callsite -- must call cold callees from here, but bottom up inlining won't see 'ptr''s value set.</div><div> }</div><div> }</div><div><br></div><div> b(void (*ptr)()) {</div><div> ptr();</div><div> }</div><div><br></div><div><br></div><div>In this example, the cold callsite 'b' may get inlined into 'a', which blocks it from being inlined into 'x'.</div><div><br></div><div>David</div><div><br></div><div> </div></div><br></div></div>