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

I recently have been running into the issue that many of these APIs perform the
deletion operation themselves and notify the caller it is going to delete
instead of allowing the caller to specify how the instruction is deleted. This
causes interesting semantic issues (see the loop in deleteInstruction I
simplified) and breaks composition since many parts of the optimizer use
InstModCallbacks for this purpose.

To fix this, I added a notify will be deleted construct to InstModCallback. In a
similar way to the rest of it, if the notify is not set, we do not call any code
implying that we should have good predictable performance in loops since we will
always skip the function call.

I also changed InstModCallback::deleteInst() to notify before deleting so we
have a default safe behavior. All previous use sites of this API do not care
about being notified and the only new use sites of this API are in
InstructionDeleter that perform special notification behavior (it notifies for
certain sets of instructions it is going to delete before it deletes any of
them). To work around this, I added a bool to deleteInst to control this
behavior and defaulted to notifying. This should ensure that all other use sites
still compose correctly.

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