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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:132
+      OldDest->removePredecessor(BB, false, &MaybeDeadInstrs);
+      RecursivelyDeleteTriviallyDeadInstructionsPermissive(MaybeDeadInstrs,
+                                                           TLI);
----------------
IIUC this function only deletes dead values if `DeleteDeadConditions` is true. Might be good to guard the new simplifications in a similar fashion.


================
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)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80206