[PATCH] D29767: GraphTraits: Add range versions of graph traits functions (graph_nodes, graph_children, inverse_graph_nodes, inverse_graph_children).

[Daniel Berlin via llvm-commits]

On Thu, Feb 9, 2017 at 12:09 PM, Tim Shen <timshen at google.com> wrote:

> On Thu, Feb 9, 2017 at 10:15 AM Daniel Berlin <dberlin at dberlin.org> wrote:
>
>> On Thu, Feb 9, 2017 at 10:01 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>
>>
>> On Thu, Feb 9, 2017 at 9:37 AM Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>> On Thu, Feb 9, 2017 at 9:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Feb 9, 2017 at 8:14 AM Daniel Berlin via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>> dberlin created this revision.
>> Herald added a reviewer: tstellarAMD.
>> Herald added subscribers: nhaehnle, arsenm.
>>
>> Convert all obvious node_begin/node_end and child_begin/child_end
>> pairs to range based for.
>>
>> Sending for review in case someone has a good idea how to make
>> graph_children able to be inferred. It looks like it would require
>> changing GraphTraits to be two argument or something. I presume
>> inference does not happen because it would have to check every
>> GraphTraits in the world to see if the noderef types matched.
>>
>>
>> Yep, that. One could use an extra trait for the node type to map it back
>> to the graph trait to facilitate this - but that'd also mean each node type
>> would have to be unique to the graph type (which is probably true - but one
>> could imagine different graphs over the same nodes? Maybe... maybe not)
>>
>>
>>
>> We have different graphs over the same nodes already.
>>
>> The nodes of a graph of the function *are basicblock *
>> the nodes of a graph of basicblock * are also basicblock *
>>
>> But they are not the same graph :)
>>
>> This only currently works through chicanery, IMHO. It would be nice to be
>> able to define different graphs over the same types, but i'm not up for
>> rewriting all of graph traits ATM.
>>
>>
>>
>>
>> Did you consider changing the trait so they exposed range accessors
>> directly, instead of adding utility functions?
>>
>>
>> I can't figure out a way to do this.
>>
>> The base class traits have *nothing* in them.
>>
>> This was the best i could do without "rewrite graphtraits completely".
>>
>> "graph_children(graph)" is a bit of an awkward construct
>>
>>
>> Note that if i put it in the traits, i believe this would get worse, not
>> matter.
>>
>>
>> How so? Worse in terms of implementation complexity, or user complexity?
>>
>>
>> User complexity. You would have to call it through graph traits, so it
>> would make user complexity worse.
>> Unless i'm missing something?
>>
>>
>> ting an undesirable asymmetry perhaps?)
>>
>>
>>
>>
>> . Alternatively maybe drop the graph_ prefix - ADL/overload
>> resolution/etc will make it correct anyway, so just "children(graph)", etc?
>>
>>
>> I'm happy to drop the prefix.
>>
>>
>>
>> (re: your other comment, I'm not sure I quite followed:
>>
>> "Note that this use of const auto & is required to make clang build, as
>> the iterator type it for CFG blocks ends up returning blocks directly
>> otherwise here." - not sure I quite follow. Best guess/reading is that the
>> CFG's iterators' value type is heavyweight, but most other GraphTraits
>> types have lightweight nodes that are cheap to copy? (like pointers, etc)
>>
>>
>> Sorta.
>>
>> First, graph traits doesn't work on anything but pointers.
>>
>>
>> Ah, I thought Tim (added Tim Shen to this thread) made this not be the
>> case anymore recently, I forget what the motivation was, and maybe it
>> didn't end up being needed/done.
>>
>>
>> Yes, sorry, It now works on references, but still not value types.
>>
>
> NodeRef as value type? I created a trait type with NodeRef as a pair, see
> include/llvm/Analysis/LoopIterator.h.
>
>

Interesting.
I tried this before and couldn't get it to work.
Now i'm going to go stare ;)

Maybe it was in the middle of your patch set or something.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/65bfa4d6/attachment.html>