[Local] Fix removeUnreachableBlocks change reporting

[jroelofs]

Summary

I believe this might be the cause of the assertion failures in the EXPENSIVE_CHECKS bot on GreenDragon. See build #16770. I can't reproduce that failure myself, but this ought to be more precise, even if it's not what's triggering that particular issue.

[fhahn]

I think it would be great if we would have a reproducer to see exactly what's going on.

If the block is already set for deletion, whoever added it should take care of dropping references & co and there should be nothing to do for us here.

So it might be more straight forward to not include blocks already pending deletion in DeadBlockSet. That way, we could have an early bail out if the deadblockset is empty. Otherwise we know we are going to make changes (mark a new block for deletion at least) and we can always return true at that point I think.

Technically, the function might not actually delete any blocks there and then, but the blocks marked for deletion here will eventually be deleted (Once the DTU flushes the updates).

[jroelofs]

I found a way to repro it, and this patch doesn't have an effect on it.

[fhahn]

In D83908#2156167, @jroelofs wrote:

I found a way to repro it, and this patch doesn't have an effect on it.

Great, thanks for confirming. As mentioned earlier, we shouldn't have to make any changes for blocks already marked as deleted. It still might be worthwhile to simplify the code as suggested.

llvm/lib/Transforms/Utils/Local.cpp

         Updates.push_back({DominatorTree::Delete, BB, Successor});
     }
     BB->dropAllReferences();
     if (DTU) {
       Instruction *TI = BB->getTerminator();
       assert(TI && "Basic block should have a terminator");
       // Terminators like invoke can have users. We have to replace their users,
       // before removing them.
-      if (!TI->use_empty())
+      if (!TI->use_empty()) {
         TI->replaceAllUsesWith(UndefValue::get(TI->getType()));
-      TI->eraseFromParent();
-      new UnreachableInst(BB->getContext(), BB);
+        Changed = true;
+      }
+      if (!isa<UnreachableInst>(TI)) {
+        TI->eraseFromParent();
+        new UnreachableInst(BB->getContext(), BB);
+        Changed = true;
+      }
       assert(succ_empty(BB) && "The successor list of BB isn't empty before "
                                "applying corresponding DTU updates.");
     }
   }
 
   if (DTU) {
     DTU->applyUpdatesPermissive(Updates);
     bool Deleted = false;
     for (auto *BB : DeadBlockSet) {
       if (DTU->isBBPendingDeletion(BB))
         --NumRemoved;
       else
         Deleted = true;
       DTU->deleteBB(BB);
     }
     if (!Deleted)
-      return false;
+      return Changed;
   } else {
     for (auto *BB : DeadBlockSet)
       BB->eraseFromParent();
   }
 
   return true;
 }