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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.

LGTM



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2318
+  // some assertion failures.
+  OI->invalidateBlock(CurrentBlock);
   CurInst->eraseFromParent();
----------------
mkazantsev wrote:
> reames wrote:
> > Caught one last bug.  The invalidation needs to be conditional since the rebuild is.  As currently structured, this will leave the entry empty allowing a miscompile.
> > 
> > Do we have a test case for the scalarPRE invalidation?  If so, why didn't it catch this?  If not, please add one.
> It is not a bug. Here we invalidate OrderedInstructions for this block because we have just erased an instruction. We should do it for all instructions, not only for implicit control flow. The re-fill of the implicit control flow map below is conditional because we only need it for ICF instructions. So at my point, it's OK.
You are correct.  I misread the code (again).


https://reviews.llvm.org/D37460