This is an archive of the discontinued LLVM Phabricator instance.

GraphTraits: Add range versions of graph traits functions (graph_nodes, graph_children, inverse_graph_nodes, inverse_graph_children).
ClosedPublic

Authored by dberlin on Feb 9 2017, 8:14 AM.

Details

Summary

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.

Note: This change was 3-staged with clang as well, which uses
Dominators/etc from LLVM.

Event Timeline

dberlin created this revision.Feb 9 2017, 8:14 AM
dberlin added inline comments.Feb 9 2017, 8:29 AM
include/llvm/Support/GenericDomTreeConstruction.h
203

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.

I'm not sure this is a legal definition of graphtraits, and the definition of CFG inverse graph traits is also definitely broken:

 template <> struct GraphTraits<Inverse< ::clang::CFGBlock*> > {
   typedef ::clang::CFGBlock *NodeRef;
-  typedef ::clang::CFGBlock::const_pred_iterator ChildIteratorType;
+  typedef ::clang::CFGBlock::pred_iterator ChildIteratorType;

(the constant iterator is being used for the non-constant version of inverse graphtraits, which looks like a paste error)

I don't usually work in clang world, so i added someone who does :)

dblaikie accepted this revision.Feb 9 2017, 10:22 AM

Looks good - sounds like you answered your own main question: no way to find the graph from the node type, so the children ops do need an explicit template param.

& I'd probably drop the "graph_" prefix - up to you though if you think that's better or worse, etc.

lib/Target/Hexagon/HexagonBitSimplify.cpp
264

I probably wouldn't bother with the extra parens there:

*DTN->getBlock()

rather than:

*(DTN->getBlock())
This revision is now accepted and ready to land.Feb 9 2017, 10:22 AM
  • Drop graph_ prefix
This revision was automatically updated to reflect the committed changes.