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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D65020#1630448 <https://reviews.llvm.org/D65020#1630448>, @efriedma wrote:

> Given that each block only has a single predecessor, there aren't any PHI nodes involved.  In that case, what does PHITranslateWithInsertion actually do?


I think this is related to how PHITranslateAddr does the translation: it only translates along CFG edges. When called with `(%input, %BB, %PredBB`), it tries to generate a value that represents `%input` in `%PredBB`. If `%input` (or any of its operands) are not defined in `%BB`, there is nothing to translate for this edge (they must also be available in the predecessor) and it will return the untranslated version of the input. This is the problem we hit in `phi_trans6`, if we would just call it with `(%gep.1, %header, %latch)`.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1204
+      // trying to number them.
+      NewInsts.pop_back_val()->eraseFromParent();
     }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65020