[PATCH] D38619: [GVN] Prevent ScalarPRE from hoisting across instructions that don't pass control flow to successors

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2210
+    if (!IsSafeToSpeculativelyExecute && FirstImplicitControlFlowInsts.count(P))
+      return false;
     // We're not interested in PRE where blocks with predecessors that are
----------------
mkazantsev wrote:
> efriedma wrote:
> > Err, wait, sorry, I wasn't reading carefully enough.
> > 
> > What are you trying to check for here?  I can't see why it matters if "P" has an implicit control flow instruction; we insert the new instruction at the end of the predecessor, so implicit control flow isn't a problem unless the terminator is an invoke.
> You're right, I misinterpreted this part. Invoke is also not a problem because if the terminator was an invoke, then we have a critical edge, and we don't PRE across critical edges without splitting them. This check is obsolete, I'll remove it.
It also seems that it's OK to do it if we don't need to insert a new instruction, i.e. when `NumWithout == 0`.


https://reviews.llvm.org/D38619