[PATCH] D80206: [IR] Clean up dead instructions after simplifying a conditional branch

[Jay Foad via Phabricator via llvm-commits]

foad marked 2 inline comments as done.
foad added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:132
+      OldDest->removePredecessor(BB, false, &MaybeDeadInstrs);
+      RecursivelyDeleteTriviallyDeadInstructionsPermissive(MaybeDeadInstrs,
+                                                           TLI);
----------------
fhahn wrote:
> IIUC this function only deletes dead values if `DeleteDeadConditions` is true. Might be good to guard the new simplifications in a similar fashion.
Under the same flag or a different one? In any case my preference would be not to do this unless/until we really need it.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:476
   for (; S != E; ++S) {
-    auto *I = cast<Instruction>(DeadInsts[S]);
-    if (!isInstructionTriviallyDead(I)) {
+    auto *I = dyn_cast<Instruction>(DeadInsts[S]);
+    if (!I || !isInstructionTriviallyDead(I)) {
----------------
fhahn wrote:
> foad wrote:
> > fhahn wrote:
> > > Why change this to a dyn_cast? Looks like only Instructions are added in `removePredecessor`?
> > `removePredecessor` only adds Instructions, but then in some cases it will `replaceAllUsesWith` one of those Instructions with an arbitrary Value.
> Hm, it seems a bit unfortunate that we would add instructions that we immediately RAUW afterwards. 
> 
> IIUC the only case where this would happen is where the incoming value for the removed predecessor is the original PN itself. Couldn't we simply not add the incoming instruction to DeadInsts? That seems like a cleaner solution and should be straight-forward.
> IIUC the only case where this would happen is where the incoming value for the removed predecessor is the original PN itself.

No, I think the problem occurs when we first visit PHI node A, one of whose inputs is PHI node B, which we add to MaybeDeadInstrs; and then we visit PHI node B and find that it hasConstantValue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80206