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

[Daniel Berlin via llvm-commits]

On Sat, Oct 1, 2016, 9:36 AM David Callahan <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.

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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161001/aa315293/attachment-0001.html>