[PATCH] D23843: GVN-hoist: fix hoistingFromAllPaths for loops (PR29034)

[Daniel Berlin via llvm-commits]

On Wed, Aug 24, 2016 at 8:17 PM, Sebastian Pop <sebpop at gmail.com> wrote:

> sebpop added inline comments.
>
> ================
> Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:315
> @@ -303,1 +314,3 @@
> +        return false;
> +
>        // Increment DFS traversal when not skipping children.
> ----------------
> dberlin wrote:
> > How is this different than just checking whether one of the successors
> is a loop backedge?
> > (and if so, why not just compute and store that set once :P)
> >
> > If it's different, how is this not basically a dominance frontier check?
> >
> > dominance frontier of BB = blocks where BB's dominance ends.
> >
> > So this is a check if any of the blocks are in the dominance frontier of
> BB, no?
> >
> >
> I think checking for loop latch is in part correct: there may be
> irreducible loops that would not be in LoopInfo and there may still exit
> backedges that would violate the property of execution on all paths.
>


Okay.


>
> Yes, the check can be rewritten as whether a successor of a block in the
> dominance frontier of BB dominates BB.  So we would not have to compute
> this property for all blocks dominated by BB, only for the blocks on the
> dominance frontier.
>
>
See if it's worth it. Storing the DF is N^2, even though it's linear time
to compute, so if doing it your current way is faster, let's go with that.

I will update the patch.  Thanks for the suggestion.
>
>
>
> https://reviews.llvm.org/D23843
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160824/4ed1e882/attachment.html>