[PATCH] D35391: [Dominators] Teach LoopDeletion to use the new incremental API

[Davide Italiano via llvm-commits]

lgtm with tobias comments addressed.

On Aug 2, 2017 9:44 AM, "Tobias Grosser via Phabricator via llvm-commits" <
llvm-commits at lists.llvm.org> wrote:

> grosser accepted this revision.
> grosser added a comment.
> This revision is now accepted and ready to land.
>
> Hi @kuhar,
>
> thank you for updating the comment. I found a couple of minor typos. The
> code likely looks good, but I don't see any reference to the irreducible
> control flow issues you mentioned. I assume you are now connecting the loop
> header to its exit to avoid the PDT update issue before. It might make
> sense to briefly explain why first deleting and then adding a new edge will
> break.
>
> In any case, these are at most minor issues. Feel free to commit.
>
> Best,
> Tobias
>
>
>
> ================
> Comment at: include/llvm/Support/GenericDomTree.h:492
> +  /// the same time, as the incremental algorithm doesn't walk the tree
> above
> +  /// the NearestCommonDominator of a deleted edge
>    ///
> ----------------
> That looks good to me.
>
>
> ================
> Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:248
> +  // Connect the preheader to the exit block. Keep the old edge to the
> header
> +  // around to perform dominator tree update in two separate steps --
> insertion
> +  // of the edge preheader->exit and deletion of the edge
> preheader->header.
> ----------------
> THE dominator tree update
>
>
> ================
> Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:260
> +  // Remove the old branch. The conditional branch becomes a new
> terminator.
> +  OldBr->eraseFromParent();
>
> ----------------
> Could you maybe move the "  DT.insertEdge(Preheader, ExitBlock);" this
> makes clear that the DT update comes right after the CFG change. Now it is
> hidden after the PHI update.
>
>
> ================
> Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:297
>
> -    // Remove the block from the reference counting scheme, so that we can
> -    // delete it freely later.
> -    (*LI)->dropAllReferences();
> -  }
> +  // Inform the domiantor tree about the removed edge.
> +  DT.deleteEdge(Preheader, L->getHeader());
> ----------------
> dominator
>
>
> ================
> Comment at: unittests/IR/DominatorTreeTest.cpp:538
> +
> +  // It is possible to perform a multiple deletions and
> +  // inform the DominatorTree about them of them at the same time, if the
> all of
> ----------------
> Remove "a"
>
>
> ================
> Comment at: unittests/IR/DominatorTreeTest.cpp:539
> +  // It is possible to perform a multiple deletions and
> +  // inform the DominatorTree about them of them at the same time, if the
> all of
> +  // the deletions happen in different subtrees.
> ----------------
> Remove "the"
> Remove "of them"
>
>
> https://reviews.llvm.org/D35391
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170802/9337d312/attachment.html>