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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1202
       if (MD) MD->removeInstruction(I);
-      I->eraseFromParent();
+      eraseFromParent(I);
     }
----------------
dberlin wrote:
> Please don't do this.
> Just mark them all for deletion, unless there is a very good reason not to.
> 
On the bug it sounded like you think not erasing immediately causes further problems.
Could you expand a bit?
It's hard to see how that could cause failures with this one since we would erase them after processing the current instruction.

The one in performScalarPRE, it looks like instructions marked to be erased are not erased after performing scalar PRE because it's done after GVN.  



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2303
   DEBUG(verifyRemoved(CurInst));
-  CurInst->eraseFromParent();
-  OI->invalidateBlock(CurrentBlock);
+  eraseFromParent(CurInst);
   ++NumGVNInstr;
----------------
dberlin wrote:
> Ditto
I can see how this may cause assertion failures (though it's silly that we do it this way), so you may have to leave this as a direct deleteInstruction call


https://reviews.llvm.org/D38944