<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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="gmail-">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="gmail-"><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><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><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="gmail-"><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="gmail-m_-4669895982744239676HOEnZb"><div class="gmail-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><div>Again, in this patch, that's not true ;)</div><div>This patch uses it in one place: lib/Analysis/SyntheticCountsUtils.cpp</div><div><br></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>The other functions in CGT (and similarly in GT) have good answers to these questions.</div><div>I'm still struggling to understand the good answer here.</div><div>I'm not suggesting there isn't one, i'm suggesting "i haven't seen/heard it so far".</div><div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span class="gmail-"><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"><div class="gmail-m_-4669895982744239676HOEnZb"><div class="gmail-m_-4669895982744239676h5">
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D42311" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4231<wbr>1</a><br>
<br>
<br>
<br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>