<br><br><div class="gmail_quote"><div dir="ltr">On Sat, Oct 1, 2016, 9:36 AM David Callahan <<a href="mailto:dcallahan@fb.com">dcallahan@fb.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">david2050 added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
> dberlin wrote in ADCE.cpp:217<br class="gmail_msg">
> I don't think this does what you think.<br class="gmail_msg">
> The order here is not actually guaranteed to be in any particular order, AFAIK.<br class="gmail_msg">
><br class="gmail_msg">
> It happens most things produce blocks in lexical order, but this is not guaranteed (again, AFAIK)<br class="gmail_msg">
><br class="gmail_msg">
> Thus, it's entirely possible you see a successor before a predecessor, which you will incorrectly mark a a back edge.<br class="gmail_msg">
> Please just use a standard DFS approach that is guaranteed correct.<br class="gmail_msg">
<br class="gmail_msg">
This seems super simple and conservative in the sense it will catch all loops. </blockquote></div><div><br></div><div><br></div><div>No actually, it will operate completely inconsistently unless bitcode guarantees an order.</div><div><br></div><div>That is, the same set of basic blocks, laid out in the file differently, will produce different results here depending how close to postorder it ends up being.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 class="gmail_msg"></blockquote></div><div><br></div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
> dberlin wrote in ADCE.cpp:228<br class="gmail_msg">
> This  isn't correct.  What if you have two backedges going out of a single block?<br class="gmail_msg">
<br class="gmail_msg">
Still can only mark the terminator of that block live once.<br class="gmail_msg"></blockquote></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
> dberlin wrote in ADCE.cpp:235<br class="gmail_msg">
> We should just fix IDF calculator in the reverse case?<br class="gmail_msg">
><br class="gmail_msg">
> In any case, this is just a reverse DFS (IE DFS over Inverse<BasicBlock>) from all blocks with no successors (LLVM does not hvae a single return block, you can't really say "the return of the function").<br class="gmail_msg">
><br class="gmail_msg">
> Please just do that.<br class="gmail_msg">
<br class="gmail_msg">
I think you are proposing to indentify all blocks where there is a path to  function return. Given that we we can then infer those blocks which do not reach a return. But this seems to be exactly the information already available from the post-dominator tree.<br class="gmail_msg"></blockquote></div><div>If true, why are you walking all the blocks and edges instead of just the blocks?</div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
> dberlin wrote in ADCE.cpp:248<br class="gmail_msg">
> This has the same bug as above :)<br class="gmail_msg">
<br class="gmail_msg">
I don't think so: marking the terminator of the source block not the successor block.<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D24918" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D24918</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>