<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 22, 2018 at 8:09 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Jan 22, 2018 at 7:48 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Jan 22, 2018 at 7:33 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="m_6580389382326261716m_7236386592907036349gmail-">On Mon, Jan 22, 2018 at 5:20 PM, Daniel Berlin 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">dberlin added a comment. <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Is it a thing that is even different for each callgraph implementation? (If not, why is it a trait?)<br></blockquote><div><br></div></span><div>My understanding is that it is different -- at least between regular callgraph and summary based callgraph used in ThinLTO (thinlink) phase.</div><span class="m_6580389382326261716m_7236386592907036349gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Even if it is, if you look, you'll see we don't try to encode random computed Node property differences in GraphTraits. Only the things that *every* graph algorithm needs (IE it caters to anything, not everything).    Random node property differences are handled through other trait classes or filtering.  This does not, at a glance, seem like a thing that literally every algorithm that is going to use a Callgraph needs. It looks like one that some may need.  You even prove this yourself.<br>
<br></blockquote><div><br></div></span><div>I see it differently -- this property does not look like some random property, but something native to callgraph nodes.</div></div></div></div></blockquote><div><br></div></span><div>Is it really native to all possible types of CallGraphs and nodes?<br>I've dealt with CallGraphs with fake nodes for exception targets where it wouldn't make any sense, for example.</div><div><br></div><div>IE Why is it so common that it should be part of the traits of all callgraphs?</div><div>and not for example, FunctionOnlyCallGraphTraits?</div><span><div><br><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="m_6580389382326261716m_7236386592907036349gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If you want to do something different here for CallGraphTraits, i'd like to understand what advantage it has.  At least in this patch as currently written, there is no advantage to has_function_def existing as a CallGraphTrait.<br>
It would work just existing fine outside of CallGraphTraits, and even as a templated function  local to a single file.<br>
<div class="m_6580389382326261716m_7236386592907036349gmail-m_-4669895982744239676HOEnZb"><div class="m_6580389382326261716m_7236386592907036349gmail-m_-4669895982744239676h5"><br></div></div></blockquote><div><br></div></span><div>This is not limited to single file, but shared across module callgraph and thinLink CG.</div></div></div></div></blockquote><div><br></div></span><div>Again, in this patch, that's not true ;)</div><div>This patch uses it in one place: lib/Analysis/SyntheticC<wbr>ountsUtils.cpp</div><div><br></div></div></div></div></blockquote><div><br></div></span><div>There are other potential uses. For instance if we want to implement a global inliner in the thinLink phase, the CGT would need to have that interface exposed.</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>There, the way it is used is not something that needs to be part of CGT at all.</div><div>Even if you shared it across other things in other functions,  why is it *so* common that literally every CGT based graph algorithm wants it, and every CGT implementation needs to implement it, and not something that is just a templated function in a header somewhere.</div><div><br></div></div></div></div></blockquote><div><br></div></span><div>An explicit specialization of CGT that is not used by any algorithm that requires checking the property does not really need to declare/implement the interface.  </div><span class="HOEnZb"><font color="#888888"><div><span style="color:rgb(34,34,34)"> </span><br></div></font></span></div></div></div></blockquote><div>Bingo, that's why it should not be in CGT unless the vast majority of algorithms need it.</div><div><br></div><div>You don't want to have a CGT that people  only implement parts of - that's a recipe for a mess.</div><div><br></div><div><br></div></div></div></div>