[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:2318
+  // some assertion failures.
+  OI->invalidateBlock(CurrentBlock);
   CurInst->eraseFromParent();
----------------
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.


https://reviews.llvm.org/D37460