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

[Xinliang David Li via llvm-commits]

On Mon, Jan 22, 2018 at 7:48 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> 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 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.


> 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.
>
>
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.

David



> 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/00e06642/attachment.html>