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

[Xinliang David Li via llvm-commits]

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

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


Alright -- I will leave this to Easwaran to prove or disapprove further if
this property  is general enough :)

David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180122/4ada9eaa/attachment.html>