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

[Sebastian Pop via llvm-commits]

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.

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.

I will update the patch.  Thanks for the suggestion.



https://reviews.llvm.org/D23843