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

[David Blaikie via llvm-commits]

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

Ah, right, I see what you mean.


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

Ah, yeah, I think Tim had talked about whether it'd be worth having the
trait operations take the key 'graph' object as a parameter too - so you
could have thin nodes that were just tokens you handed back to the graph to
do anything with them. Don't think he ended up implementing that but
instead hiding a pointer to the graph in every node handle, maybe.


>
> 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/37b59299/attachment.html>