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

[David Callahan via llvm-commits]

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. 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.

> 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