[PATCH] D42311: [SyntheticCounts] Rewrite the code using only graph traits.

[Daniel Berlin via llvm-commits]

On Mon, Jan 22, 2018 at 7:33 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Mon, Jan 22, 2018 at 5:20 PM, Daniel Berlin via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> dberlin added a comment.
>>
> Is it a thing that is even different for each callgraph implementation?
>> (If not, why is it a trait?)
>>
>
> My understanding is that it is different -- at least between regular
> callgraph and summary based callgraph used in ThinLTO (thinlink) phase.
>
>
>> 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.
>>
>>
> I see it differently -- this property does not look like some random
> property, but something native to callgraph nodes.
>

Is it really native to all possible types of CallGraphs and nodes?
I've dealt with CallGraphs with fake nodes for exception targets where it
wouldn't make any sense, for example.

IE Why is it so common that it should be part of the traits of all
callgraphs?
and not for example, FunctionOnlyCallGraphTraits?




>
>
>> 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.
>> It would work just existing fine outside of CallGraphTraits, and even as
>> a templated function  local to a single file.
>>
>>
> This is not limited to single file, but shared across module callgraph and
> thinLink CG.
>

Again, in this patch, that's not true ;)
This patch uses it in one place: lib/Analysis/SyntheticCountsUtils.cpp

There, the way it is used is not something that needs to be part of CGT at
all.
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.

The other functions in CGT (and similarly in GT) have good answers to these
questions.
I'm still struggling to understand the good answer here.
I'm not suggesting there isn't one, i'm suggesting "i haven't seen/heard it
so far".




>
> David
>
>
>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D42311
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180122/1c3c705c/attachment.html>