[silgen-cleanup] Ensure that we move nextII past /all/ instructions we are going to delete no matter order of visitation.

Specifically, given code like the following:

```
(%1, %2) = multi_result_inst %0   (a)
inst_to_delete %1                 (b)
inst_to_delete %2                 (c)
```

We would sometimes visit (c) before (b). So if nextII was (b) at that point, we
would think that we were safe. Then we process (b), move nextII from (b) ->
(c). Then we delete both (b) and (c). =><=.

The solution to this is each iteration of the delete loop, we track /all/
instructions that eliminateDeadInstruction is going to eliminate and ensure that
after each callback is called we have moved past all visited instructions (even
ones we visited previously). This is done using a small set that I clear each
iteration.

I also re-enabled the dictionary test that exposed this issue when testing
-O/-Osize.

rdar://71933996

Author: gottesmm

lib/SILOptimizer/Mandatory/SILGenCleanup.cpp

@@ -16,6 +16,7 @@
 
 #define DEBUG_TYPE "silgen-cleanup"
 
+#include "swift/Basic/Defer.h"
 #include "swift/SIL/BasicBlockUtils.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
@@ -48,16 +49,54 @@ struct SILGenCanonicalize final : CanonicalizeInstruction {
 
   void notifyHasNewUsers(SILValue) override { changed = true; }
 
-  SILBasicBlock::iterator deleteDeadOperands(SILBasicBlock::iterator nextII) {
-    // Delete trivially dead instructions in non-determistic order.
+  /// Delete trivially dead instructions in non-determistic order.
+  ///
+  /// We either have that nextII is endII or if nextII is not endII then endII
+  /// 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;
+
     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);
-        if (nextII == deadInst->getIterator())
+
+        // 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);
       });
     }
@@ -97,7 +136,7 @@ void SILGenCleanup::run() {
     for (auto &bb : function) {
       for (auto ii = bb.begin(), ie = bb.end(); ii != ie;) {
         ii = sgCanonicalize.canonicalize(&*ii);
-        ii = sgCanonicalize.deleteDeadOperands(ii);
+        ii = sgCanonicalize.deleteDeadOperands(ii, ie);
       }
     }
     if (sgCanonicalize.changed) {

test/SILOptimizer/silgen_cleanup.sil

@@ -216,3 +216,26 @@ bb0(%0 : $*X3, %1 : @guaranteed $Builtin.NativeObject):
   %12 = tuple ()
   return %12 : $()
 }
+
+// We used to hit a memory error on this test.
+//
+// CHECK-LABEL: sil [ossa] @testDestructureTupleNoCrash : $@convention(thin) (@owned (Builtin.NativeObject, Builtin.NativeObject)) -> () {
+// CHECK: bb0(
+// CHECK-NEXT: destroy_value
+// CHECK-NEXT: tuple
+// CHECK-NEXT: return
+// CHECK: } // end sil function 'testDestructureTupleNoCrash'
+sil [ossa] @testDestructureTupleNoCrash : $@convention(thin) (@owned (Builtin.NativeObject, Builtin.NativeObject)) -> () {
+bb0(%0 : @owned $(Builtin.NativeObject, Builtin.NativeObject)):
+  (%1, %2) = destructure_tuple %0 : $(Builtin.NativeObject, Builtin.NativeObject)
+  debug_value %1 : $Builtin.NativeObject, let, name "key"
+  debug_value %2 : $Builtin.NativeObject, let, name "value"
+  %3 = begin_borrow %1 : $Builtin.NativeObject
+  end_borrow %3 : $Builtin.NativeObject
+  %4 = begin_borrow %2 : $Builtin.NativeObject
+  end_borrow %4 : $Builtin.NativeObject
+  destroy_value %2 : $Builtin.NativeObject
+  destroy_value %1 : $Builtin.NativeObject
+  %9999 = tuple()
+  return %9999 : $()
+}

validation-test/stdlib/Dictionary.swift

@@ -7,10 +7,6 @@
 // RUN: %target-codesign %t/Dictionary && %line-directive %t/main.swift -- %target-run %t/Dictionary
 // REQUIRES: executable_test
 
-// rdar71933996
-// UNSUPPORTED: swift_test_mode_optimize
-// UNSUPPORTED: swift_test_mode_optimize_size
-
 import StdlibUnittest
 import StdlibCollectionUnittest