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

[Daniel Berlin via llvm-commits]

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

>
>
> 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.
>
>
Bingo, that's why it should not be in CGT unless the vast majority of
algorithms need it.

You don't want to have a CGT that people  only implement parts of - that's
a recipe for a mess.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180122/d9f9c541/attachment.html>