Merge pull request #37081 from gottesmm/pr-f4e3be62c879d615aa05c8181a481e36205ea258

[sil-optimizer] Fix feedback from #37016 and finish removing confusing constructors

Author: gottesmm

include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

@@ -46,17 +46,22 @@ struct CanonicalizeInstruction {
   CanonicalizeInstruction(const char *passDebugType,
                           DeadEndBlocks &deadEndBlocks)
       : deadEndBlocks(deadEndBlocks),
-        callbacks(
-            [&](SILInstruction *toDelete) { killInstruction(toDelete); },
-            [&](SILInstruction *newInst) { notifyNewInstruction(newInst); },
-            [&](Operand *use, SILValue newValue) {
-              use->set(newValue);
-              notifyHasNewUsers(newValue);
-            }) {
+        callbacks() {
 #ifndef NDEBUG
     if (llvm::DebugFlag && !llvm::isCurrentDebugType(debugType))
       debugType = passDebugType;
 #endif
+    callbacks = InstModCallbacks()
+      .onDelete([&](SILInstruction *toDelete) {
+        killInstruction(toDelete);
+      })
+      .onCreateNewInst([&](SILInstruction *newInst) {
+        notifyNewInstruction(newInst);
+      })
+      .onSetUseValue([&](Operand *use, SILValue newValue) {
+        use->set(newValue);
+        notifyHasNewUsers(newValue);
+      });
   }
 
   virtual ~CanonicalizeInstruction();

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -48,11 +48,6 @@ template <class T> class NullablePtr;
 /// that in a loop this property should predict well since you have a single
 /// branch that is going to go the same way everytime.
 class InstModCallbacks {
-  /// A function that takes in an instruction and deletes the inst.
-  ///
-  /// Default implementation is instToDelete->eraseFromParent();
-  std::function<void(SILInstruction *instToDelete)> deleteInstFunc;
-
   /// A function that is called to notify that a new function was created.
   ///
   /// Default implementation is a no-op, but we still mark madeChange.
@@ -66,37 +61,47 @@ class InstModCallbacks {
   /// iterators.
   std::function<void(Operand *use, SILValue newValue)> setUseValueFunc;
 
+  /// A function that takes in an instruction and deletes the inst.
+  ///
+  /// Default implementation is instToDelete->eraseFromParent();
+  ///
+  /// NOTE: The reason why we have deleteInstFunc and notifyWillBeDeletedFunc is
+  /// InstModCallback supports 2 stage deletion where a callee passed
+  /// InstModCallback is allowed to drop all references to the instruction
+  /// before calling deleteInstFunc. In contrast, notifyWillBeDeletedFunc
+  /// assumes that the IR is in a good form before being called so that the
+  /// caller can via the callback gather state about the instruction that will
+  /// be deleted. As an example, see InstructionDeleter::deleteInstruction() in
+  /// InstOptUtils.cpp.
+  std::function<void(SILInstruction *instToDelete)> deleteInstFunc;
+
   /// If non-null, called before an instruction is deleted or has its references
-  /// dropped.
+  /// dropped. If null, no-op.
+  ///
+  /// NOTE: The reason why we have deleteInstFunc and notifyWillBeDeletedFunc is
+  /// InstModCallback supports 2 stage deletion where a callee passed
+  /// InstModCallback is allowed to drop all references to the instruction
+  /// before calling deleteInstFunc. In contrast, notifyWillBeDeletedFunc
+  /// assumes that the IR is in a good form before being called so that the
+  /// caller can via the callback gather state about the instruction that will
+  /// be deleted. As an example, see InstructionDeleter::deleteInstruction() in
+  /// InstOptUtils.cpp.
+  ///
+  /// NOTE: This is called in InstModCallback::deleteInst() if one does not use
+  /// a default bool argument to disable the notification. In general that
+  /// should only be done when one is writing custom handling and is performing
+  /// the notification ones self. It is assumed that the notification will be
+  /// called with a valid instruction.
   std::function<void(SILInstruction *instThatWillBeDeleted)>
       notifyWillBeDeletedFunc;
 
   /// A boolean that tracks if any of our callbacks were ever called.
   bool wereAnyCallbacksInvoked = false;
 
 public:
-  InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc)
-      : deleteInstFunc(deleteInstFunc) {}
-
-  InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
-                   decltype(createdNewInstFunc) createdNewInstFunc)
-      : deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc) {
-  }
-
-  InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
-                   decltype(setUseValueFunc) setUseValueFunc)
-      : deleteInstFunc(deleteInstFunc), setUseValueFunc(setUseValueFunc) {}
-
-  InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
-                   decltype(createdNewInstFunc) createdNewInstFunc,
-                   decltype(setUseValueFunc) setUseValueFunc)
-      : deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc),
-        setUseValueFunc(setUseValueFunc) {}
-
   InstModCallbacks() = default;
   ~InstModCallbacks() = default;
   InstModCallbacks(const InstModCallbacks &) = default;
-  InstModCallbacks(InstModCallbacks &&) = default;
 
   /// Return a copy of self with deleteInstFunc set to \p newDeleteInstFunc.
   InstModCallbacks onDelete(decltype(deleteInstFunc) newDeleteInstFunc) const {

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

@@ -143,18 +143,22 @@ class MandatoryCombiner final
                     DeadEndBlocks &deadEndBlocks)
       : compilingWithOptimization(optimized), worklist("MC"), madeChange(false),
         iteration(0),
-        instModCallbacks(
-            [&](SILInstruction *instruction) {
-              worklist.erase(instruction);
-              instructionsPendingDeletion.push_back(instruction);
-            },
-            [&](SILInstruction *instruction) { worklist.add(instruction); },
-            [this](Operand *use, SILValue newValue) {
-              use->set(newValue);
-              worklist.add(use->getUser());
-            }),
+        instModCallbacks(),
         createdInstructions(createdInstructions),
-        deadEndBlocks(deadEndBlocks){};
+        deadEndBlocks(deadEndBlocks) {
+    instModCallbacks = InstModCallbacks()
+      .onDelete([&](SILInstruction *instruction) {
+        worklist.erase(instruction);
+        instructionsPendingDeletion.push_back(instruction);
+      })
+      .onCreateNewInst([&](SILInstruction *instruction) {
+        worklist.add(instruction);
+      })
+      .onSetUseValue([this](Operand *use, SILValue newValue) {
+        use->set(newValue);
+        worklist.add(use->getUser());
+      });
+  };
 
   void addReachableCodeToWorklist(SILFunction &function);
 

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

@@ -527,7 +527,7 @@ static bool stripOwnership(SILFunction &func) {
     auto value = visitor.instructionsToSimplify.pop_back_val();
     if (!value.hasValue())
       continue;
-    InstModCallbacks callbacks([&](SILInstruction *instToErase) {
+    auto callbacks = InstModCallbacks().onDelete([&](SILInstruction *instToErase) {
       visitor.eraseInstruction(instToErase);
     });
     // We are no longer in OSSA, so we don't need to pass in a deBlocks.

lib/SILOptimizer/SILCombiner/SILCombiner.h

@@ -116,19 +116,21 @@ class SILCombiner :
             },
             /* EraseAction */
             [&](SILInstruction *I) { eraseInstFromFunction(*I); }),
-        instModCallbacks(
-            [&](SILInstruction *instToDelete) {
-              eraseInstFromFunction(*instToDelete);
-            },
-            [&](SILInstruction *newlyCreatedInst) {
-              Worklist.add(newlyCreatedInst);
-            },
-            [&](Operand *use, SILValue newValue) {
-              use->set(newValue);
-              Worklist.add(use->getUser());
-            }),
+        instModCallbacks(),
         deBlocks(&B.getFunction()),
-        ownershipFixupContext(instModCallbacks, deBlocks) {}
+        ownershipFixupContext(instModCallbacks, deBlocks) {
+    instModCallbacks = InstModCallbacks()
+      .onDelete([&](SILInstruction *instToDelete) {
+              eraseInstFromFunction(*instToDelete);
+      })
+      .onCreateNewInst([&](SILInstruction *newlyCreatedInst) {
+        Worklist.add(newlyCreatedInst);
+      })
+      .onSetUseValue([&](Operand *use, SILValue newValue) {
+        use->set(newValue);
+        Worklist.add(use->getUser());
+      });
+  }
 
   bool runOnFunction(SILFunction &F);
 

lib/SILOptimizer/SemanticARC/SemanticARCOptVisitor.h

@@ -52,9 +52,10 @@ struct LLVM_LIBRARY_VISIBILITY SemanticARCOptVisitor
   explicit SemanticARCOptVisitor(SILFunction &fn, DeadEndBlocks &deBlocks,
                                  bool onlyMandatoryOpts)
       : ctx(fn, deBlocks, onlyMandatoryOpts,
-            InstModCallbacks(
-                [this](SILInstruction *inst) { eraseInstruction(inst); },
-                [this](Operand *use, SILValue newValue) {
+            InstModCallbacks()
+                .onDelete(
+                    [this](SILInstruction *inst) { eraseInstruction(inst); })
+                .onSetUseValue([this](Operand *use, SILValue newValue) {
                   use->set(newValue);
                   worklist.insert(newValue);
                 })) {}

lib/SILOptimizer/Transforms/TempRValueElimination.cpp

@@ -783,7 +783,7 @@ void TempRValueOptPass::run() {
     }
   }
 
-  InstModCallbacks callbacks(
+  auto callbacks = InstModCallbacks().onDelete(
     [](SILInstruction *instToKill) {
       // SimplifyInstruction is not in the business of removing
       // copy_addr. If it were, then we would need to update deadCopies.