<div dir="ltr">(and as for whether to use po_iterator, yes, we don't have a good dfs visitation scheme.<div>Unless you want to write better iterators with more control right now,  I would just make a simple backedge discovery routine that uses a custom worklist DFS.</div><div>GenericDomTreeConstruction.h has one you can copy, as does df_iterator.)</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Oct 1, 2016 at 2:05 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin added inline comments.<br>
<br>
<br>
> david2050 wrote in ADCE.cpp:217<br>
> This seems super simple and conservative in the sense it will catch all loops. The variant of this code before this review cycle started used po_iterator but there no measurable benefit but quite a bit more code.<br>
<br>
So, here are two of identical code, laid out in the IR differently,  where we will do different things, and should not.<br>
<br>
A<br>
jump C<br>
B<br>
jump D<br>
C<br>
jump B<br>
<br>
A<br>
jump C<br>
C<br>
jump B<br>
B<br>
jump D<br>
<br>
In the first, the visitation order will be A, B, C.<br>
You will mark A as seen.<br>
No successors seen, nothing marked.<br>
You will now mark B as visited<br>
No successors seen, nothing marked<br>
You will now mark C as visited<br>
You will see that you have marked B as visited<br>
You will incorrectly think this is a back edge.<br>
<br>
In the second, the visitation order, will be A, C, B.<br>
You will mark A as seen.<br>
No successors seen, nothing marked.<br>
You will mark C as seen<br>
No successors seen, nothing marked.<br>
You will mark B as seen<br>
No successors seen, nothing marked<br>
<br>
> david2050 wrote in ADCE.cpp:228<br>
<span class="">> Still can only mark the terminator of that block live once.<br>
<br>
</span>yes, i read it wrong, sorry!<br>
<br>
> david2050 wrote in ADCE.cpp:248<br>
<span class="">> I don't think so: marking the terminator of the source block not the successor block.<br>
<br>
</span>yes, i read it wrong, sorry!<br>
<br>
<a href="https://reviews.llvm.org/D24918" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D24918</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>