[PATCH] D24918: [ADCE] Add code to remove dead branches

[Mehdi Amini via llvm-commits]

> On Oct 1, 2016, at 9:22 AM, Daniel Berlin <dberlin at dberlin.org> wrote:
> 
> 
> 
> On Sat, Oct 1, 2016, 9:36 AM David Callahan <dcallahan at fb.com <mailto:dcallahan at fb.com>> wrote:
> david2050 added inline comments.
> 
> 
> > dberlin wrote in ADCE.cpp:217
> > I don't think this does what you think.
> > The order here is not actually guaranteed to be in any particular order, AFAIK.
> >
> > It happens most things produce blocks in lexical order, but this is not guaranteed (again, AFAIK)
> >
> > Thus, it's entirely possible you see a successor before a predecessor, which you will incorrectly mark a a back edge.
> > Please just use a standard DFS approach that is guaranteed correct.
> 
> This seems super simple and conservative in the sense it will catch all loops. 
> 
> 
> No actually, it will operate completely inconsistently unless bitcode guarantees an order.

Order of basic-blocks is guaranteed when round-tripping to bitcode (We really try to avoid relying on other *use-list* though).

To be clear: I’m not saying that is great to rely on the block order either, I just wanted to clarify the bitcode guarantees :)

— 
Mehdi



> 
> 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.
> 
> The variant of this code before this review cycle started used po_iterator but there no measurable benefit but quite a bit more code.
> 
> 
> 
> 
> > dberlin wrote in ADCE.cpp:228
> > This  isn't correct.  What if you have two backedges going out of a single block?
> 
> Still can only mark the terminator of that block live once.
> 
> 
> > dberlin wrote in ADCE.cpp:235
> > We should just fix IDF calculator in the reverse case?
> >
> > 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").
> >
> > Please just do that.
> 
> 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.
> If true, why are you walking all the blocks and edges instead of just the blocks?
> 
> 
> 
> > dberlin wrote in ADCE.cpp:248
> > This has the same bug as above :)
> 
> I don't think so: marking the terminator of the source block not the successor block.
> 
> https://reviews.llvm.org/D24918 <https://reviews.llvm.org/D24918>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161001/c1689afe/attachment.html>