[PATCH] D38944: [GVN] Handle removal of first implicit CF instruction correctly

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1202
       if (MD) MD->removeInstruction(I);
-      I->eraseFromParent();
+      assert(FirstImplicitControlFlowInsts.lookup(I->getParent()) != I &&
+             "Newly created instruction has implicit control flow?");
----------------
This assertion doesn't look correct.  Your message says the instruction doesn't have implicit control flow, the actual assert says it's not the first in the basic block.  One or the other needs fixed.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1204
+             "Newly created instruction has implicit control flow?");
+      markInstructionForDeletion(I);
     }
----------------
This looks highly suspicious.  Why remove from MD, but not erase?  I think you want to either a) remove the MD->removeInstruction (because it's handled on actual deletion), or b) leave a good comment here.

Also, if replacing eraseInstruction with markForDeletion really is NFC, it should be a standalone patch which this is rebased on.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2104
+    bool InvalidateImplicitCF = false;
+    for (auto *I : InstrsToErase) {
+      assert(I->getParent() == BB && "Erasing instruction from wrong block?");
----------------
The switch to a range iteration, and addition of the assertion is a separable change.  Please commit that as a change of it's own and rebase the patch to reduce the diff for review.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2109
+      DEBUG(verifyRemoved(I));
+      if (!InvalidateImplicitCF &&
+          FirstImplicitControlFlowInsts.lookup(I->getParent()) == I)
----------------
This can be written as:
InvalidateImplicitCF |= new_condition;


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2313
   DEBUG(verifyRemoved(CurInst));
+  bool InvalidateImplicitCF =
+      FirstImplicitControlFlowInsts.lookup(CurInst->getParent()) == CurInst;
----------------
Hm, optional larger code organization suggestion.  Could we introduce two operations: one which invalidates and one which rebuilds?  This would make the invariants of the FillICFI function more clear and would make this code a bit more readable.

If we could arrange to have the ICFI lazily rebuild on access to the block, that might be an overall cleaner scheme.

Again, both parts are optional.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2408
+    // Make sure that the instruction is not marked for further removal.
+    assert(std::find(InstrsToErase.begin(), InstrsToErase.end(), I) ==
+           InstrsToErase.end() && "Filling before removed all marked insns?");
----------------
This assert doesn't look like it belongs in the lamda.  Lift out to containing function.


https://reviews.llvm.org/D38944