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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1202
       if (MD) MD->removeInstruction(I);
-      I->eraseFromParent();
+      eraseFromParent(I);
     }
----------------
dberlin wrote:
> 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.  
> 
No matter when we erase, we should update the implicit CF map after we do it. When we used `OrderedInstructions` we assumed that the invariant "first instruction with implicit CF" preserves, no matter what happens. But it appeared that it doesn't (for example, if there are two calls into `readnone` function with the same args in different blocks; then the dominated call can be eliminated).

Even if we mark for deletion, we should update the map immediately to be able to hoist across the marked instruction. Marking alone  does not solve the problem, it will still need the same code that also updates the map. If marking is only needed for uniformity of GVN code, this can be done separately from fixing the actual bug.



https://reviews.llvm.org/D38944