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

So if i want a completely virtual graph that i make up as it calls child
iterators, for example, i can't really give it that (even after Tim's
patches, it doesn't work).

I have to have created it beforehand in memory.

That said, it's also really hard to do a virtual graph anyway because it
uses static functions, which means literally no state.
Such is life.



>
>>   Which is very very sad, but also outside the scope of this patch (and
>> also means you can't, for example, have a graph of pairs or wahtever).
>> Some day i will fix this.
>>
>> In any case, this particular issue is:
>>
>> the child iterators are expected to return noderefs, but it looks like
>> the iterator for clang's CFG returns a value type.
>>
>> IE the iterators return, say, BasicBlock * (a pointer type).
>> Clang's returns AdjacentCFGBlock  (a value type).
>>
>
> I'm confused then - sounds like a contradiction, that CFG does have a
> NodeRef that's not a pointer.
>
> *goes to look* Ah, I see, the NodeRef is a pointer type, but the child
> dereference doesn't produce that, instead it produces something that's
> convertible to a pointer. It's not much bigger - it's two pointers big
> instead of one pointer.
>

Right.


>
> And... I see what/why it's doing. Yeah, that. (ramble ramble) Yeah,
> wouldn't be hard to add an iterator adapter in there to force the
> conversion so its op*/value_type is /actually/ CFGBlock* instead of
> something convertible to it.
>

Yes.


>
>
>>
>> Thus, if i use "auto *" instead of "const auto &", it fails.
>>
>
> Right - from the comment I was reading it as "using const auto & instead
> of auto" - wasn't thinking about "const auto *" as the alternative that was
> being compared/contrasted.
>
>
>> I believe clang is broken here, but honestly, it's hard to tell.
>>
>
> Fair.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/ab254202/attachment.html>