Merge pull request #37016 from gottesmm/pr-eb1b4b484d8990f6f5cb6706dba952b3e27cdc59

[sil-optimizer] Make InstructionDeleter and related APIs to use an InstModCallback instead of a notification callback.

Author: gottesmm

include/swift/SILOptimizer/Utils/DebugOptUtils.h

@@ -43,6 +43,43 @@ inline void deleteAllDebugUses(SILInstruction *inst,
   }
 }
 
+/// Erases the instruction \p I from it's parent block and deletes it, including
+/// all debug instructions which use \p I.
+/// Precondition: The instruction may only have debug instructions as uses.
+/// If the iterator \p InstIter references any deleted instruction, it is
+/// incremented.
+///
+/// \p callbacks InstModCallback to use.
+///
+/// Returns an iterator to the next non-deleted instruction after \p I.
+inline SILBasicBlock::iterator
+eraseFromParentWithDebugInsts(SILInstruction *inst,
+                              InstModCallbacks callbacks = InstModCallbacks()) {
+  auto nextII = std::next(inst->getIterator());
+
+  auto results = inst->getResults();
+  bool foundAny;
+  do {
+    foundAny = false;
+    for (auto result : results) {
+      while (!result->use_empty()) {
+        foundAny = true;
+        auto *user = result->use_begin()->getUser();
+        assert(user->isDebugInstruction());
+        if (nextII == user->getIterator())
+          ++nextII;
+        callbacks.deleteInst(user);
+      }
+    }
+  } while (foundAny);
+
+  // Just matching what eraseFromParentWithDebugInsts is today.
+  if (nextII == inst->getIterator())
+    ++nextII;
+  callbacks.deleteInst(inst, false /*do not notify*/);
+  return nextII;
+}
+
 } // namespace swift
 
 #endif

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -1,4 +1,4 @@
-//===--- OSLogOptimizer.cpp - Optimizes calls to OS Log ===//
+//===--- OSLogOptimizer.cpp - Optimizes calls to OS Log -------------------===//
 //
 // This source file is part of the Swift.org open source project
 //
@@ -68,6 +68,8 @@
 /// 'substituteConstants' and 'emitCodeForSymbolicValue'. The remaining
 /// functions in the file implement the subtasks and utilities needed by the
 /// above functions.
+///
+//===----------------------------------------------------------------------===//
 
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/DiagnosticEngine.h"
@@ -1108,8 +1110,6 @@ static bool checkOSLogMessageIsConstant(SingleValueInstruction *osLogMessage,
   return errorDetected;
 }
 
-using CallbackTy = llvm::function_ref<void(SILInstruction *)>;
-
 /// Return true iff the given address-valued instruction has only stores into
 /// it. This function tests for the conditions under which a call, that was
 /// constant evaluated, that writes into the address-valued instruction can be
@@ -1184,8 +1184,7 @@ static bool hasOnlyStoreUses(SingleValueInstruction *addressInst) {
 /// of begin_apply. This will also fix the lifetimes of the deleted instructions
 /// whenever possible.
 static void forceDeleteAllocStack(SingleValueInstruction *inst,
-                                  InstructionDeleter &deleter,
-                                  CallbackTy callback) {
+                                  InstructionDeleter &deleter) {
   SmallVector<SILInstruction *, 8> users;
   for (Operand *use : inst->getUses())
     users.push_back(use->getUser());
@@ -1194,30 +1193,31 @@ static void forceDeleteAllocStack(SingleValueInstruction *inst,
     if (isIncidentalUse(user))
       continue;
     if (isa<DestroyAddrInst>(user)) {
-      deleter.forceDelete(user, callback);
+      deleter.forceDelete(user);
       continue;
     }
     if (isa<BeginAccessInst>(user)) {
-      forceDeleteAllocStack(cast<BeginAccessInst>(user), deleter, callback);
+      forceDeleteAllocStack(cast<BeginAccessInst>(user), deleter);
       continue;
     }
-    deleter.forceDeleteAndFixLifetimes(user, callback);
+    deleter.forceDeleteAndFixLifetimes(user);
   }
-  deleter.forceDelete(inst, callback);
+  deleter.forceDelete(inst);
 }
 
 /// Delete \c inst , if it is dead, along with its dead users and invoke the
 /// callback whever an instruction is deleted.
-static void deleteInstructionWithUsersAndFixLifetimes(
-    SILInstruction *inst, InstructionDeleter &deleter, CallbackTy callback) {
+static void
+deleteInstructionWithUsersAndFixLifetimes(SILInstruction *inst,
+                                          InstructionDeleter &deleter) {
   // If this is an alloc_stack, it can be eliminated as long as it is only
   // stored into or destroyed.
   if (AllocStackInst *allocStack = dyn_cast<AllocStackInst>(inst)) {
     if (hasOnlyStoreUses(allocStack))
-      forceDeleteAllocStack(allocStack, deleter, callback);
+      forceDeleteAllocStack(allocStack, deleter);
     return;
   }
-  deleter.recursivelyDeleteUsersIfDead(inst, callback);
+  deleter.recursivelyDeleteUsersIfDead(inst);
 }
 
 /// Try to dead-code eliminate the OSLogMessage instance \c oslogMessage passed
@@ -1226,31 +1226,35 @@ static void deleteInstructionWithUsersAndFixLifetimes(
 /// \returns true if elimination is successful and false if it is not successful
 /// and diagnostics is emitted.
 static bool tryEliminateOSLogMessage(SingleValueInstruction *oslogMessage) {
-  InstructionDeleter deleter;
   // List of instructions that are possibly dead.
   SmallVector<SILInstruction *, 4> worklist = {oslogMessage};
   // Set of all deleted instructions.
   SmallPtrSet<SILInstruction *, 4> deletedInstructions;
+
+  auto callbacks =
+      InstModCallbacks().onNotifyWillBeDeleted([&](SILInstruction *deadInst) {
+        // Add operands of all deleted instructions to the worklist so that
+        // they can be recursively deleted if possible.
+        for (Operand &operand : deadInst->getAllOperands()) {
+          if (SILInstruction *definingInstruction =
+                  operand.get()->getDefiningInstruction()) {
+            if (!deletedInstructions.count(definingInstruction))
+              worklist.push_back(definingInstruction);
+          }
+        }
+        (void)deletedInstructions.insert(deadInst);
+      });
+  InstructionDeleter deleter(callbacks);
+
   unsigned startIndex = 0;
   while (startIndex < worklist.size()) {
     SILInstruction *inst = worklist[startIndex++];
     if (deletedInstructions.count(inst))
       continue;
-    deleteInstructionWithUsersAndFixLifetimes(
-        inst, deleter, [&](SILInstruction *deadInst) {
-          // Add operands of all deleted instructions to the worklist so that
-          // they can be recursively deleted if possible.
-          for (Operand &operand : deadInst->getAllOperands()) {
-            if (SILInstruction *definingInstruction =
-                    operand.get()->getDefiningInstruction()) {
-              if (!deletedInstructions.count(definingInstruction))
-                worklist.push_back(definingInstruction);
-            }
-          }
-          (void)deletedInstructions.insert(deadInst);
-        });
+    deleteInstructionWithUsersAndFixLifetimes(inst, deleter);
   }
   deleter.cleanUpDeadInstructions();
+
   // If the OSLogMessage instance is not deleted, either we couldn't see the
   // body of the log call or there is a bug in the library implementation.
   // Assuming that the library implementation is correct, it means that either

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -55,50 +55,27 @@ struct SILGenCanonicalize final : CanonicalizeInstruction {
   /// is nextII->getParent()->end().
   SILBasicBlock::iterator deleteDeadOperands(SILBasicBlock::iterator nextII,
                                              SILBasicBlock::iterator endII) {
-    // Each iteration, we store the instructions that will be deleted in the
-    // iteration here and use it to ensure that nextII is moved past /all/
-    // instructions that we are going to delete no matter the order (since we
-    // are visiting instructions in non-deterministic order).
-    SmallPtrSet<SILInstruction *, 16> willBeDeletedInIteration;
+    auto callbacks = InstModCallbacks().onDelete([&](SILInstruction *deadInst) {
+      LLVM_DEBUG(llvm::dbgs() << "Trivially dead: " << *deadInst);
+
+      // If nextII is the instruction we are going to delete, move nextII past
+      // it.
+      if (deadInst->getIterator() == nextII)
+        ++nextII;
+
+      // Then remove the instruction from the set and delete it.
+      deadOperands.erase(deadInst);
+      deadInst->eraseFromParent();
+    });
 
     while (!deadOperands.empty()) {
       SILInstruction *deadOperInst = *deadOperands.begin();
 
       // Make sure at least the first instruction is removed from the set.
       deadOperands.erase(deadOperInst);
 
-      // Then add our initial instruction to the will be deleted set.
-      willBeDeletedInIteration.insert(deadOperInst);
-      SWIFT_DEFER { willBeDeletedInIteration.clear(); };
-
-      eliminateDeadInstruction(deadOperInst, [&](SILInstruction *deadInst) {
-        LLVM_DEBUG(llvm::dbgs() << "Trivially dead: " << *deadInst);
-
-        // Add our instruction to the will be deleted set.
-        willBeDeletedInIteration.insert(deadInst);
-
-        // Then look through /all/ instructions that we are going to delete in
-        // this iteration until we hit the end of the list. This ensures that in
-        // a situation like the following:
-        //
-        // ```
-        //   (%1, %2) = multi_result_inst %0   (a)
-        //   inst_to_delete %1                 (b)
-        //   inst_to_delete %2                 (c)
-        // ```
-        //
-        // If nextII is on (b), but we visit (c) before visiting (b), then we
-        // will end up with nextII on (c) after we are done and then delete
-        // (c). In contrast by using the set when we process (b) after (c), we
-        // first see that (b) is nextII [since it is in the set] so move to (c)
-        // and then see that (c) is in the set as well (since we inserted it
-        // previously) and skip that.
-        while (nextII != endII && willBeDeletedInIteration.count(&*nextII))
-          ++nextII;
-
-        // Then remove the instruction from the set.
-        deadOperands.erase(deadInst);
-      });
+      // Then delete this instruction/everything else that we can.
+      eliminateDeadInstruction(deadOperInst, callbacks);
     }
     return nextII;
   }

lib/SILOptimizer/Mandatory/SILGenCleanup.cpp

@@ -1727,6 +1727,13 @@ ConstantFolder::processWorkList() {
                           I->eraseFromParent();
                         });
 
+  auto callbacks =
+      InstModCallbacks().onDelete([&](SILInstruction *instToDelete) {
+        WorkList.remove(instToDelete);
+        InvalidateInstructions = true;
+        instToDelete->eraseFromParent();
+      });
+
   // An out parameter array that we use to return new simplified results from
   // constantFoldInstruction.
   SmallVector<SILValue, 8> ConstantFoldedResults;
@@ -1736,8 +1743,6 @@ ConstantFolder::processWorkList() {
 
     LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *I);
 
-    Callback(I);
-
     // Replace assert_configuration instructions by their constant value. We
     // want them to be replace even if we can't fully propagate the constant.
     if (AssertConfiguration != SILOptions::DisableReplacement)
@@ -1751,29 +1756,28 @@ ConstantFolder::processWorkList() {
           // Schedule users for constant folding.
           WorkList.insert(AssertConfInt);
           // Delete the call.
-          eliminateDeadInstruction(BI);
-
-          InvalidateInstructions = true;
+          eliminateDeadInstruction(BI, callbacks);
           continue;
         }
 
         // Kill calls to conditionallyUnreachable if we've folded assert
         // configuration calls.
         if (isApplyOfBuiltin(*BI, BuiltinValueKind::CondUnreachable)) {
           assert(BI->use_empty() && "use of conditionallyUnreachable?!");
-          recursivelyDeleteTriviallyDeadInstructions(BI, /*force*/ true);
+          recursivelyDeleteTriviallyDeadInstructions(BI, /*force*/ true,
+                                                     callbacks);
           InvalidateInstructions = true;
           continue;
         }
       }
+
     // Replace a known availability.version semantic call.
     if (isApplyOfKnownAvailability(*I)) {
       if (auto apply = dyn_cast<ApplyInst>(I)) {
         SILBuilderWithScope B(I);
         auto tru = B.createIntegerLiteral(apply->getLoc(), apply->getType(), 1);
         apply->replaceAllUsesWith(tru);
-        eliminateDeadInstruction(
-            I, [&](SILInstruction *DeadI) { WorkList.remove(DeadI); });
+        eliminateDeadInstruction(I, callbacks);
         WorkList.insert(tru);
         InvalidateInstructions = true;
       }
@@ -1822,9 +1826,7 @@ ConstantFolder::processWorkList() {
       if (constantFoldGlobalStringTablePointerBuiltin(cast<BuiltinInst>(I),
                                                       EnableDiagnostics)) {
         // Here, the bulitin instruction got folded, so clean it up.
-        eliminateDeadInstruction(
-            I, [&](SILInstruction *DeadI) { WorkList.remove(DeadI); });
-        InvalidateInstructions = true;
+        eliminateDeadInstruction(I, callbacks);
       }
       continue;
     }
@@ -1839,10 +1841,8 @@ ConstantFolder::processWorkList() {
           auto *cfi = builder.createCondFail(I->getLoc(), I->getOperand(0),
                                              sli->getValue());
           WorkList.insert(cfi);
-          recursivelyDeleteTriviallyDeadInstructions(
-              I, /*force*/ true,
-              [&](SILInstruction *DeadI) { WorkList.remove(DeadI); });
-          InvalidateInstructions = true;
+          recursivelyDeleteTriviallyDeadInstructions(I, /*force*/ true,
+                                                     callbacks);
         }
       }
       continue;
@@ -1851,10 +1851,8 @@ ConstantFolder::processWorkList() {
     if (isApplyOfBuiltin(*I, BuiltinValueKind::IsConcrete)) {
       if (constantFoldIsConcrete(cast<BuiltinInst>(I))) {
         // Here, the bulitin instruction got folded, so clean it up.
-        recursivelyDeleteTriviallyDeadInstructions(
-            I, /*force*/ true,
-            [&](SILInstruction *DeadI) { WorkList.remove(DeadI); });
-        InvalidateInstructions = true;
+        recursivelyDeleteTriviallyDeadInstructions(I, /*force*/ true,
+                                                   callbacks);
       }
       continue;
     }
@@ -1875,7 +1873,7 @@ ConstantFolder::processWorkList() {
     }
 
     // Go through all users of the constant and try to fold them.
-    InstructionDeleter deleter;
+    InstructionDeleter deleter(callbacks);
     for (auto Result : I->getResults()) {
       for (auto *Use : Result->getUses()) {
         SILInstruction *User = Use->getUser();
@@ -2022,12 +2020,10 @@ ConstantFolder::processWorkList() {
         }
       }
     }
+
     // Eagerly DCE. We do this after visiting all users to ensure we don't
     // invalidate the uses iterator.
-    deleter.cleanUpDeadInstructions([&](SILInstruction *DeadI) {
-      WorkList.remove(DeadI);
-      InvalidateInstructions = true;
-    });
+    deleter.cleanUpDeadInstructions();
   }
 
   // TODO: refactor this code outside of the method. Passes should not merge