[addr-move-function] Fix a bug where I was inferring a value from certain destroy_addr to insert compensating destroy_addr.

This doesn't work well if one can potentially not have any input destroy_addr...
I actually already know the address without inferring it, so I changed the code
to just use that.

I also added more tests around defer/without defer that exercise this behavior.

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp

@@ -1364,7 +1364,7 @@ struct DataflowState {
   bool handleSingleBlockClosure(SILArgument *address,
                                 ClosureOperandState &state);
   bool cleanupAllDestroyAddr(
-      SILFunction *fn, SmallBitVector &destroyIndices,
+      SILValue address, SILFunction *fn, SmallBitVector &destroyIndices,
       SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
       BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
       BasicBlockSet &blocksWithMovesThatAreNowTakes,
@@ -1383,14 +1383,14 @@ struct DataflowState {
 } // namespace
 
 bool DataflowState::cleanupAllDestroyAddr(
-    SILFunction *fn, SmallBitVector &destroyIndices,
+    SILValue address, SILFunction *fn, SmallBitVector &destroyIndices,
     SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
     BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
     BasicBlockSet &blocksWithMovesThatAreNowTakes,
     SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
   bool madeChange = false;
   BasicBlockWorklist worklist(fn);
-  SILValue daiOperand;
+
   LLVM_DEBUG(llvm::dbgs() << "Cleanup up destroy addr!\n");
   LLVM_DEBUG(llvm::dbgs() << "    Visiting destroys!\n");
   LLVM_DEBUG(llvm::dbgs() << "    Destroy Indices: " << destroyIndices << "\n");
@@ -1401,9 +1401,6 @@ bool DataflowState::cleanupAllDestroyAddr(
     if (!dai)
       continue;
     LLVM_DEBUG(llvm::dbgs() << "    Destroy: " << *dai);
-    SILValue op = (*dai)->getOperand();
-    assert(daiOperand == SILValue() || op == daiOperand);
-    daiOperand = op;
     for (auto *predBlock : (*dai)->getParent()->getPredecessorBlocks()) {
       worklist.pushIfNotVisited(predBlock);
     }
@@ -1415,7 +1412,7 @@ bool DataflowState::cleanupAllDestroyAddr(
     auto reinit = useState.reinits[index];
     if (!reinit)
       continue;
-    LLVM_DEBUG(llvm::dbgs() << "  Reinit: " << *reinit);
+    LLVM_DEBUG(llvm::dbgs() << "  Reinit: " << **reinit);
     for (auto *predBlock : (*reinit)->getParent()->getPredecessorBlocks()) {
       worklist.pushIfNotVisited(predBlock);
     }
@@ -1436,16 +1433,33 @@ bool DataflowState::cleanupAllDestroyAddr(
   while (auto *next = worklist.pop()) {
     LLVM_DEBUG(llvm::dbgs()
                << "Looking at block: bb" << next->getDebugID() << "\n");
+
     // Any blocks that contained processed moves are stop points.
-    if (blocksWithMovesThatAreNowTakes.contains(next))
+    if (blocksWithMovesThatAreNowTakes.contains(next)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "    Block contained a move that is now a true take.\n");
       continue;
+    }
 
+    // Then if we find that we have a block that was never visited when we
+    // walked along successor edges from the move, then we know that we need to
+    // insert a destroy_addr.
+    //
+    // This is safe to do since this block lives along the dominance frontier
+    // and we do not allow for critical edges, so as we walk along predecessors,
+    // given that any such block must also have a successor that was reachable
+    // from our move, we know that this unprocessed block must only have one
+    // successor, a block reachable from our move and thus must not have any
+    // unhandled uses.
     if (!blocksVisitedWhenProcessingNewTakes.contains(next)) {
+      LLVM_DEBUG(llvm::dbgs() << "    Found a block that was not visited when "
+                                 "we processed takes of the given move.\n");
       // Insert a destroy_addr here since the block isn't reachable from any of
       // our moves.
-      SILBuilderWithScope builder(next->getTerminator());
+      SILBuilderWithScope builder(
+          std::prev(next->getTerminator()->getIterator()));
       auto *dvi = builder.createDestroyAddr(
-          RegularLocation::getAutoGeneratedLocation(), daiOperand);
+          RegularLocation::getAutoGeneratedLocation(), address);
       useState.destroys.insert(dvi);
       continue;
     }
@@ -1538,7 +1552,7 @@ bool DataflowState::process(
     return indicesOfPairedConsumingClosureUses;
   };
 
-  BasicBlockSet visitedByNewMove(fn);
+  BasicBlockSet blocksVisitedWhenProcessingNewTakes(fn);
   BasicBlockSet blocksWithMovesThatAreNowTakes(fn);
   bool convertedMarkMoveToTake = false;
   for (auto *mvi : markMovesThatPropagateDownwards) {
@@ -1547,12 +1561,12 @@ bool DataflowState::process(
     LLVM_DEBUG(llvm::dbgs() << "Checking Multi Block Dataflow for: " << *mvi);
 
     BasicBlockWorklist worklist(fn);
-    BasicBlockSetVector setVector(fn);
+    BasicBlockSetVector visitedBlocks(fn);
     for (auto *succBlock : mvi->getParent()->getSuccessorBlocks()) {
       LLVM_DEBUG(llvm::dbgs()
                  << "    SuccBlocks: " << succBlock->getDebugID() << "\n");
       worklist.pushIfNotVisited(succBlock);
-      setVector.insert(succBlock);
+      visitedBlocks.insert(succBlock);
     }
 
     while (auto *next = worklist.pop()) {
@@ -1684,7 +1698,7 @@ bool DataflowState::process(
       // Otherwise, add successors if we haven't visited them to the worklist.
       for (auto *succBlock : next->getSuccessorBlocks()) {
         worklist.pushIfNotVisited(succBlock);
-        setVector.insert(succBlock);
+        visitedBlocks.insert(succBlock);
       }
     }
 
@@ -1701,8 +1715,8 @@ bool DataflowState::process(
       builder.createCopyAddr(mvi->getLoc(), mvi->getSrc(), mvi->getDest(),
                              IsTake, IsInitialization);
       // Flush our SetVector into the visitedByNewMove.
-      for (auto *block : setVector) {
-        visitedByNewMove.insert(block);
+      for (auto *block : visitedBlocks) {
+        blocksVisitedWhenProcessingNewTakes.insert(block);
       }
       convertedMarkMoveToTake = true;
     }
@@ -1716,9 +1730,10 @@ bool DataflowState::process(
   // Now that we have processed all of our mark_moves, eliminate all of the
   // destroy_addr.
   madeChange |= cleanupAllDestroyAddr(
-      fn, getIndicesOfPairedDestroys(), getIndicesOfPairedReinits(),
-      getIndicesOfPairedConsumingClosureUses(), visitedByNewMove,
-      blocksWithMovesThatAreNowTakes, postDominatingConsumingUsers);
+      address, fn, getIndicesOfPairedDestroys(), getIndicesOfPairedReinits(),
+      getIndicesOfPairedConsumingClosureUses(),
+      blocksVisitedWhenProcessingNewTakes, blocksWithMovesThatAreNowTakes,
+      postDominatingConsumingUsers);
 
   return madeChange;
 }

test/SILOptimizer/move_function_kills_copyable_addressonly_vars.swift

@@ -23,7 +23,12 @@ func exchangeUse<T>(_ k: __owned T) -> T { k }
 
 public protocol P {
     var k: Klass { get }
+
+    static func getP() -> Self
+
+    func doSomething()
 }
+
 public protocol SubP1 : P {}
 public protocol SubP2 : P {}
 
@@ -214,10 +219,7 @@ struct S<T> {
 // Defer Tests //
 /////////////////
 
-protocol DeferTestProtocol {
-    var k: Klass { get }
-
-    static func getP() -> Self
+protocol DeferTestProtocol : P {
 }
 
 extension DeferTestProtocol {
@@ -413,6 +415,37 @@ extension DeferTestProtocol {
         }
         print("foo bar")
     }
+
+    mutating func deferTestSuccess13() {
+        let selfType = type(of: self)
+        if booleanValue {
+            print("creating blocks")
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        defer {
+            self = selfType.getP()
+        }
+        print("foo bar")
+    }
+
+    mutating func deferTestSuccess14() {
+        let selfType = type(of: self)
+        if booleanValue {
+            print("creating blocks")
+            self.doSomething()
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        defer {
+            self = selfType.getP()
+        }
+        print("foo bar")
+    }
 }
 
 ////////////////
@@ -553,3 +586,48 @@ public func partialApplyTest<T : P>(_ x: __owned T) {
     }
     f()
 }
+
+////////////////////////
+// Misc Tests on Self //
+////////////////////////
+
+protocol MiscTests : P {}
+
+extension MiscTests {
+
+    // This test makes sure that we are able to properly put in the destroy_addr
+    // in the "creating blocks" branch. There used to be a bug where the impl
+    // would need at least one destroy_addr to properly infer the value to put
+    // into blocks not reachable from the _move but that are on the dominance
+    // frontier from the _move. This was unnecessary and the test makes sure we
+    // do not fail on this again.
+    mutating func noDestroyAddrBeforeOptInsertAfter() {
+        let selfType = type(of: self)
+        if booleanValue {
+            print("creating blocks")
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        self = selfType.getP()
+        print("foo bar")
+    }
+
+    // A derived version of noDestroyAddrBeforeOptInsertAfter that makes sure
+    // when we insert the destroy_addr, we destroy self at the end of the block.
+    mutating func noDestroyAddrBeforeOptInsertAfter2() {
+        let selfType = type(of: self)
+        if booleanValue {
+            print("creating blocks")
+            self.doSomething()
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        self = selfType.getP()
+        print("foo bar")
+    }
+}
+

test/SILOptimizer/move_function_kills_copyable_loadable_vars.swift

@@ -463,6 +463,35 @@ extension KlassWrapper {
         }
         print("foo bar")
     }
+
+    mutating func deferTestSuccess13() {
+        if booleanValue {
+            print("creating blocks")
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        defer {
+            self = KlassWrapper(k: Klass())
+        }
+        print("foo bar")
+    }
+
+    mutating func deferTestSuccess14() {
+        if booleanValue {
+            print("creating blocks")
+            self.doSomething()
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        defer {
+            self = KlassWrapper(k: Klass())
+        }
+        print("foo bar")
+    }
 }
 
 ////////////////
@@ -603,3 +632,45 @@ public func partialApplyTest(_ x: __owned Klass) {
     }
     f()
 }
+
+////////////////////////
+// Misc Tests on Self //
+////////////////////////
+
+extension KlassWrapper {
+
+    func doSomething() { print("foo") }
+
+    // This test makes sure that we are able to properly put in the destroy_addr
+    // in the "creating blocks" branch. There used to be a bug where the impl
+    // would need at least one destroy_addr to properly infer the value to put
+    // into blocks not reachable from the _move but that are on the dominance
+    // frontier from the _move. This was unnecessary and the test makes sure we
+    // do not fail on this again.
+    mutating func noDestroyAddrBeforeOptInsertAfter() {
+        if booleanValue {
+            print("creating blocks")
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        self = .init(k: Klass())
+        print("foo bar")
+    }
+
+    // A derived version of noDestroyAddrBeforeOptInsertAfter that makes sure
+    // when we insert the destroy_addr, we destroy self at the end of the block.
+    mutating func noDestroyAddrBeforeOptInsertAfter2() {
+        if booleanValue {
+            print("creating blocks")
+            self.doSomething()
+        } else {
+            let _ = _move(self)
+            print("creating blocks2")
+        }
+
+        self = .init(k: Klass())
+        print("foo bar")
+    }
+}