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

[Tim Shen via llvm-commits]

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

> On Thu, Feb 9, 2017 at 9:23 AM David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> 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)
>
> Did you consider changing the trait so they exposed range accessors
> directly, instead of adding utility functions?
>
>
> The facts that:
> 1) We don't want the user to define both begin()/end(), and the range
> interface.
> 2) Algorithms allow user to pass in their home-grown, arbitrary traits
> type, not necessarily GraphTraits.
>

these facts make having a children() helper inside traits type hard.


>
>
> Ideally I'd like to have something like this:
>
> template<typename GraphType>
> struct GraphTraitsFacade;
>
> template<>
> struct GraphTraitsFacade<MyGraph> {
>   using ChilditeratorType = ...;
>   ChildIteratorType child_begin();
>   ChildIteratorType child_end();
>   ...
> };
>
> template<typename MyGraph>
> struct GraphTraits : GraphtraitsFacade<MyGraph> {
>   static_assert(/* check for Facade conformance */, "");
>   iterator_range<ChildIteratorType> children(); // convenience helper
> };
>
> All users specialize on GraphtraitsFacade; all algorithms use GraphTraits.
> If a user want multiple graphs with the same graph type, well, that's a
> contradiction. :) the user should create a strong alias of that graph type.
>
> Oh well.
>
>
> "graph_children(graph)" is a bit of an awkward construct. Alternatively
> maybe drop the graph_ prefix - ADL/overload resolution/etc will make it
> correct anyway, so just "children(graph)", etc?
>
> (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)
>
>
>
> Note: This change was 3-staged with clang as well, which uses
> Dominators/etc from LLVM.
>
>
> https://reviews.llvm.org/D29767
>
> Files:
>   include/llvm/ADT/GraphTraits.h
>   include/llvm/Support/GenericDomTree.h
>   include/llvm/Support/GenericDomTreeConstruction.h
>   lib/Analysis/IteratedDominanceFrontier.cpp
>   lib/Target/AMDGPU/AMDILCFGStructurizer.cpp
>   lib/Target/Hexagon/HexagonBitSimplify.cpp
>   lib/Target/Hexagon/HexagonCommonGEP.cpp
>   lib/Target/Hexagon/HexagonGenExtract.cpp
>   lib/Target/Hexagon/HexagonGenInsert.cpp
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/9ae262c3/attachment.html>