[PATCH] D65020: [GVN] Do PHI translations across all edges between the load and the unavailable pred.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1204
+      // trying to number them.
+      NewInsts.pop_back_val()->eraseFromParent();
     }
----------------
fhahn wrote:
> efriedma wrote:
> > I don't see any explanation for this change?
> markInstructionForDeletion asserts that the removed instructions are defined in the current basic block. 
> 
> But PHI translation may insert instructions in any of the basic blocks traversed during translation. If that happens and translation fails, we have to remove instructions from different basic blocks than the current one. I thought it is best to directly remove them (no other parts of GVN have seen them yet and they are not numbered yet).
> 
> I can update the comment to make that clearer.
That makes sense.  Please update the comment to at least mention the cross-BB issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65020/new/

https://reviews.llvm.org/D65020