[PATCH] D37460: [GVN] Prevent LoadPRE 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:2312
   if (MD)
     MD->removeInstruction(CurInst);
   DEBUG(verifyRemoved(CurInst));
----------------
reames wrote:
> reames wrote:
> > It just occurred to me, are you sure loads - which are the only instruction type in use here - every have implicit control flow?  If not, this piece is dead code and should be replaced with an assertion instead.
> > 
> > In particular:
> > assert(!MayNotTransferExecutionToSuccessor(CurInst), "loads don't have implicit control flow");
> Talking to Max, we realized my comment here is wrong.  This is the *Scalar* PRE block, not the *Load* PRE block I thought I was looking at.
Yeah, and in general we mark all instructions for deletion (no matter loads or not) and then process them uniformly. That it is not done here is just a mess (due to some assertion failures if we do); I think it is to be fixed in future.


https://reviews.llvm.org/D37460