<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Feb 9, 2017 at 12:27 PM Tim Shen <<a href="mailto:timshen@google.com">timshen@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Feb 9, 2017 at 9:23 AM David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Feb 9, 2017 at 8:14 AM Daniel Berlin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin created this revision.<br class="gmail_msg">
Herald added a reviewer: tstellarAMD.<br class="gmail_msg">
Herald added subscribers: nhaehnle, arsenm.<br class="gmail_msg">
<br class="gmail_msg">
Convert all obvious node_begin/node_end and child_begin/child_end<br class="gmail_msg">
pairs to range based for.<br class="gmail_msg">
<br class="gmail_msg">
Sending for review in case someone has a good idea how to make<br class="gmail_msg">
graph_children able to be inferred. It looks like it would require<br class="gmail_msg">
changing GraphTraits to be two argument or something. I presume<br class="gmail_msg">
inference does not happen because it would have to check every<br class="gmail_msg">
GraphTraits in the world to see if the noderef types matched.<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">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)<br class="gmail_msg"><br class="gmail_msg">Did you consider changing the trait so they exposed range accessors directly, instead of adding utility functions?<br class="gmail_msg"></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">The facts that:</div><div class="gmail_msg">1) We don't want the user to define both begin()/end(), and the range interface.</div><div class="gmail_msg">2) Algorithms allow user to pass in their home-grown, arbitrary traits type, not necessarily GraphTraits.</div></div></div></blockquote><div><br></div><div>these facts make having a children() helper inside traits type hard.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Ideally I'd like to have something like this:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">template<typename GraphType></div><div class="gmail_msg">struct GraphTraitsFacade;</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">template<></div><div class="gmail_msg">struct GraphTraitsFacade<MyGraph> {</div><div class="gmail_msg"> using ChilditeratorType = ...;</div><div class="gmail_msg"> ChildIteratorType child_begin();</div><div class="gmail_msg"> ChildIteratorType child_end();</div><div class="gmail_msg"> ...</div><div class="gmail_msg">};</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">template<typename MyGraph></div><div class="gmail_msg">struct GraphTraits : GraphtraitsFacade<MyGraph> {</div><div class="gmail_msg"> static_assert(/* check for Facade conformance */, "");</div><div class="gmail_msg"> iterator_range<ChildIteratorType> children(); // convenience helper</div><div class="gmail_msg">};<br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Oh well.</div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">"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?<br class="gmail_msg"><br class="gmail_msg">(re: your other comment, I'm not sure I quite followed:<br class="gmail_msg"><br class="gmail_msg">"<span style="color:rgb(33,33,33);font-size:13px" class="gmail_msg">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)<br class="gmail_msg"></span></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
Note: This change was 3-staged with clang as well, which uses<br class="gmail_msg">
Dominators/etc from LLVM.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D29767" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D29767</a><br class="gmail_msg">
<br class="gmail_msg">
Files:<br class="gmail_msg">
include/llvm/ADT/GraphTraits.h<br class="gmail_msg">
include/llvm/Support/GenericDomTree.h<br class="gmail_msg">
include/llvm/Support/GenericDomTreeConstruction.h<br class="gmail_msg">
lib/Analysis/IteratedDominanceFrontier.cpp<br class="gmail_msg">
lib/Target/AMDGPU/AMDILCFGStructurizer.cpp<br class="gmail_msg">
lib/Target/Hexagon/HexagonBitSimplify.cpp<br class="gmail_msg">
lib/Target/Hexagon/HexagonCommonGEP.cpp<br class="gmail_msg">
lib/Target/Hexagon/HexagonGenExtract.cpp<br class="gmail_msg">
lib/Target/Hexagon/HexagonGenInsert.cpp<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>
_______________________________________________<br class="gmail_msg">
llvm-commits mailing list<br class="gmail_msg">
<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="gmail_msg">
</blockquote></div></div></blockquote></div></div>