[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);
----------------
foad wrote:
> 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.
It would probably make sense to use the same flag. I assume there is a reason for making removing instructions conditional on an option (which defaults to false and some users set it to false). It seems like silently changing it to delete some other bits could trip over some existing users in obscure edge cases.

It might be that none of the users actually care of course. But in that case we should remove the flag altogether. In any case, I think the behavior should be documented.


================
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:
> > 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.
Yeah that's another case, probably also possible to detect up-front. Anyways, it seems like we already allow non-instructions in DeadInsts in `RecursivelyDeleteTriviallyDeadInstructions` as well. Seems a bit odd to have something called DeadInsts containing non-instructions, but I guess that ship has sailed. Nevermind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80206