[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 requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Ok, once more very close.  Thank you for working through the issues raised and splitting out the various bits.  Now that's done, one more round of updates and we should be good for a resubmit.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2101
+    bool InvalidateImplicitCF = false;
+    const Instruction *FirstICF = FirstImplicitControlFlowInsts.lookup(BB);
     for (auto *I : InstrsToErase) {
----------------
Are we guaranteed this entry exists in the map?  Or are you relying on default construction of the value type?  I think the second, in which case it might be worth renaming the variable to something like FirstICFOrNull for clarity.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2116
+
+    if (!InstrsToErase.empty())
+      OI->invalidateBlock(BB);
----------------
This check is implied by one further up in the same scope.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2312
   if (MD)
     MD->removeInstruction(CurInst);
   DEBUG(verifyRemoved(CurInst));
----------------
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");


================
Comment at: test/Transforms/GVN/PRE/pre-load.ll:434
+
+; Don't PRE load across calls.
+
----------------
across *potentially throwing* calls


================
Comment at: test/Transforms/GVN/PRE/pre-load.ll:436
+
+define i32 @test13(i32* noalias nocapture readonly %x, i32* noalias nocapture %r, i32 %a) {
+; CHECK-LABEL: @test13(
----------------
Required - Add an analogous test case where @f is in a block other than the origin block.

Required - Add a pair of analogous test cases where %x is marked as dereferenceable to exercise the speculation logic in each case.


https://reviews.llvm.org/D37460