[move-function] Break blocks right after the non-undef debug info associated with move only values.

The reason why I am doing this is that right now SelectionDAG seems to sink
certain llvm.dbg.addr to the end of block. By breaking the block, we ensure that
we initialize the debug value of values early enough. NOTE: We are relying on
code at -Onone not eliminating control flow which it does not today.

A few additional notes:

1. I also fixed a small issue where I was not setting the proper debug scope for
reinit debug_values. This came up when I was writing test cases for this commit.
So I just fixed it at the same time.

2. Since I am splitting blocks and I don't want to invalidate dominators/etc at
this point in the pipeline, I took advantage of the APIs ability to update
dominators/loopinfo at the same time.

3. I also expanded the tests in tree further by adding debug info tests for the
reinit copyable/addressonly cases in the multi-block case.

Author: gottesmm

include/swift/SILOptimizer/Utils/CFGOptUtils.h

@@ -144,6 +144,22 @@ SILBasicBlock *splitBasicBlockAndBranch(SILBuilder &builder,
                                         DominanceInfo *domInfo,
                                         SILLoopInfo *loopInfo);
 
+/// A version of splitBasicBlockAndBranch that takes a SILBuilderContext instead
+/// of a SILBuilder. We generally are trying to eliminate APIs that take in
+/// SILBuilder directly since that can cause weird downstream mistakes around
+/// debug info scopes. So this provides a better choice for engineers.
+///
+/// TODO: Migrate all callers of splitBasicBlockAndBranch to use this entry
+/// point.
+inline SILBasicBlock *splitBasicBlockAndBranch(SILBuilderContext &builderCtx,
+                                               SILInstruction *splitBeforeInst,
+                                               DominanceInfo *domInfo,
+                                               SILLoopInfo *loopInfo) {
+  // Make sure we have the right debug scope from split before inst.
+  SILBuilderWithScope builder(splitBeforeInst, builderCtx);
+  return splitBasicBlockAndBranch(builder, splitBeforeInst, domInfo, loopInfo);
+}
+
 /// Return true if the function has a critical edge, false otherwise.
 bool hasCriticalEdges(SILFunction &f, bool onlyNonCondBr);
 

lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp

@@ -158,7 +158,9 @@
 #include "swift/SIL/SILVisitor.h"
 #include "swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h"
 #include "swift/SILOptimizer/Analysis/ClosureScope.h"
+#include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/CFGOptUtils.h"
 #include "swift/SILOptimizer/Utils/CanonicalOSSALifetime.h"
 #include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SILOptimizer/Utils/SILOptFunctionBuilder.h"
@@ -1396,17 +1398,19 @@ struct DataflowState {
         applySiteToPromotedArgIndices(applySiteToPromotedArgIndices),
         closureConsumes(closureConsumes) {}
   void init();
-  bool process(
-      SILValue address,
-      SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
+  bool
+  process(SILValue address,
+          SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
+          SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints);
   bool handleSingleBlockClosure(SILArgument *address,
                                 ClosureOperandState &state);
   bool cleanupAllDestroyAddr(
       SILValue address, SILFunction *fn, SmallBitVector &destroyIndices,
       SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
       BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
       BasicBlockSet &blocksWithMovesThatAreNowTakes,
-      SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
+      SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
+      SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints);
   void clear() {
     useBlocks.clear();
     initBlocks.clear();
@@ -1425,7 +1429,8 @@ bool DataflowState::cleanupAllDestroyAddr(
     SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
     BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
     BasicBlockSet &blocksWithMovesThatAreNowTakes,
-    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
+    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
+    SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints) {
   bool madeChange = false;
   BasicBlockWorklist worklist(fn);
 
@@ -1538,8 +1543,13 @@ bool DataflowState::cleanupAllDestroyAddr(
     if (debugVarInst) {
       if (auto varInfo = debugVarInst.getVarInfo()) {
         SILBuilderWithScope reinitBuilder(*reinit);
-        reinitBuilder.createDebugValue(debugVarInst.inst->getLoc(), address,
-                                       *varInfo, false, /*was moved*/ true);
+        reinitBuilder.setCurrentDebugScope(debugVarInst->getDebugScope());
+        auto *dvi =
+            reinitBuilder.createDebugValue(debugVarInst.inst->getLoc(), address,
+                                           *varInfo, false, /*was moved*/ true);
+        // After we are done processing, we are going to split at the reinit
+        // point.
+        debugInfoBlockSplitPoints.insert(dvi);
       }
     }
     madeChange = true;
@@ -1581,7 +1591,8 @@ bool DataflowState::cleanupAllDestroyAddr(
 
 bool DataflowState::process(
     SILValue address,
-    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
+    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
+    SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints) {
   SILFunction *fn = address->getFunction();
   assert(fn);
 
@@ -1778,9 +1789,9 @@ bool DataflowState::process(
         debug.markAsMoved();
         if (auto varInfo = debug.getVarInfo()) {
           SILBuilderWithScope undefBuilder(builder);
-          undefBuilder.setCurrentDebugScope(debug.inst->getDebugScope());
+          undefBuilder.setCurrentDebugScope(debug->getDebugScope());
           undefBuilder.createDebugValue(
-              debug.inst->getLoc(),
+              debug->getLoc(),
               SILUndef::get(address->getType(), builder.getModule()), *varInfo,
               false /*poison*/, true /*was moved*/);
         }
@@ -1800,13 +1811,25 @@ bool DataflowState::process(
   if (!convertedMarkMoveToTake)
     return madeChange;
 
+  // Now if we had a debug var carrying inst for our address, add the debug var
+  // carrying inst to debugInfoBlockSplitPoints so when we are done processing
+  // we can break blocks at those locations. This is done to ensure that we
+  // don't have to worry about the CFG changing while we are processing. The
+  // reason why we do this is that we are working around a bug in SelectionDAG
+  // that results in llvm.dbg.addr being sunk to the end of blocks. This can
+  // cause the value to appear to not be available after it is initialized. By
+  // breaking the block here, we guarantee that SelectionDAG's sinking has no
+  // effect since we are the end of the block.
+  if (auto debug = DebugVarCarryingInst::getFromValue(address))
+    debugInfoBlockSplitPoints.insert(*debug);
+
   // Now that we have processed all of our mark_moves, eliminate all of the
   // destroy_addr.
   madeChange |= cleanupAllDestroyAddr(
       address, fn, getIndicesOfPairedDestroys(), getIndicesOfPairedReinits(),
       getIndicesOfPairedConsumingClosureUses(),
       blocksVisitedWhenProcessingNewTakes, blocksWithMovesThatAreNowTakes,
-      postDominatingConsumingUsers);
+      postDominatingConsumingUsers, debugInfoBlockSplitPoints);
 
   return madeChange;
 }
@@ -1895,9 +1918,10 @@ void DataflowState::init() {
 // Returns true if we emitted a diagnostic and handled the single block
 // case. Returns false if we visited all of the uses and seeded the UseState
 // struct with the information needed to perform our interprocedural dataflow.
-static bool performSingleBasicBlockAnalysis(DataflowState &dataflowState,
-                                            SILValue address,
-                                            MarkUnresolvedMoveAddrInst *mvi) {
+static bool performSingleBasicBlockAnalysis(
+    DataflowState &dataflowState,
+    SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints,
+    SILValue address, MarkUnresolvedMoveAddrInst *mvi) {
   // First scan downwards to make sure we are move out of this block.
   auto &useState = dataflowState.useState;
   auto &applySiteToPromotedArgIndices =
@@ -1926,6 +1950,7 @@ static bool performSingleBasicBlockAnalysis(DataflowState &dataflowState,
     if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
       if (auto varInfo = debug.getVarInfo()) {
         SILBuilderWithScope undefBuilder(builder);
+        debugInfoBlockSplitPoints.insert(*debug);
         undefBuilder.setCurrentDebugScope(debug.inst->getDebugScope());
         undefBuilder.createDebugValue(
             debug.inst->getLoc(),
@@ -2036,6 +2061,7 @@ static bool performSingleBasicBlockAnalysis(DataflowState &dataflowState,
     if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
       if (auto varInfo = debug.getVarInfo()) {
         {
+          debugInfoBlockSplitPoints.insert(*debug);
           SILBuilderWithScope undefBuilder(builder);
           undefBuilder.setCurrentDebugScope(debug.inst->getDebugScope());
           undefBuilder.createDebugValue(
@@ -2049,8 +2075,11 @@ static bool performSingleBasicBlockAnalysis(DataflowState &dataflowState,
           // reinit instruction so we reshow the variable.
           auto *next = interestingUser->getNextInstruction();
           SILBuilderWithScope reinitBuilder(next);
-          reinitBuilder.createDebugValue(debug.inst->getLoc(), address,
-                                         *varInfo, false, /*was moved*/ true);
+          reinitBuilder.setCurrentDebugScope(debug->getDebugScope());
+          auto *dvi = reinitBuilder.createDebugValue(debug.inst->getLoc(),
+                                                     address, *varInfo, false,
+                                                     /*was moved*/ true);
+          debugInfoBlockSplitPoints.insert(dvi);
         }
       }
       debug.markAsMoved();
@@ -2086,6 +2115,7 @@ static bool performSingleBasicBlockAnalysis(DataflowState &dataflowState,
                dumpBitVector(llvm::dbgs(), bitVector); llvm::dbgs() << '\n');
     if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
       if (auto varInfo = debug.getVarInfo()) {
+        debugInfoBlockSplitPoints.insert(*debug);
         SILBuilderWithScope undefBuilder(builder);
         undefBuilder.setCurrentDebugScope(debug.inst->getDebugScope());
         undefBuilder.createDebugValue(
@@ -2137,6 +2167,19 @@ struct MoveKillsCopyableAddressesChecker {
       applySiteToPromotedArgIndices;
   SmallBlotSetVector<SILInstruction *, 8> closureConsumes;
 
+  /// A list of instructions where to work around the behavior of SelectionDAG,
+  /// we break the block. These are debug var carrying insts or debug_value
+  /// associated with reinits. This is initialized when we process all of the
+  /// addresses. Then as a final step after wards we use this as a worklist and
+  /// break blocks at each of these instructions. We update DebugInfo, LoopInfo
+  /// if we found that they already exist.
+  ///
+  /// We on purpose use a set vector to ensure that we only ever split a block
+  /// once.
+  SmallSetVector<SILInstruction *, 8> debugInfoBlockSplitPoints;
+  DominanceInfo *dominanceToUpdate = nullptr;
+  SILLoopInfo *loopInfoToUpdate = nullptr;
+
   MoveKillsCopyableAddressesChecker(SILFunction *fn,
                                     SILOptFunctionBuilder &funcBuilder)
       : fn(fn), useState(),
@@ -2145,6 +2188,12 @@ struct MoveKillsCopyableAddressesChecker {
         closureUseState(), closureUseDataflowState(closureUseState),
         funcBuilder(funcBuilder) {}
 
+  void setDominanceToUpdate(DominanceInfo *newInfo) {
+    dominanceToUpdate = newInfo;
+  }
+
+  void setLoopInfoToUpdate(SILLoopInfo *newInfo) { loopInfoToUpdate = newInfo; }
+
   void cloneDeferCalleeAndRewriteUses(
       SmallVectorImpl<SILValue> &temporaryStorage,
       const SmallBitVector &bitVector, FullApplySite oldApplySite,
@@ -2156,6 +2205,19 @@ struct MoveKillsCopyableAddressesChecker {
 
   void emitDiagnosticForMove(SILValue borrowedValue,
                              StringRef borrowedValueName, MoveValueInst *mvi);
+  bool splitBlocksAfterDebugInfoCarryingInst(SILModule &mod) {
+    if (debugInfoBlockSplitPoints.empty())
+      return false;
+
+    SILBuilderContext ctx(mod);
+    do {
+      auto *next = debugInfoBlockSplitPoints.pop_back_val();
+      splitBasicBlockAndBranch(ctx, next->getNextInstruction(),
+                               dominanceToUpdate, loopInfoToUpdate);
+    } while (!debugInfoBlockSplitPoints.empty());
+
+    return true;
+  }
 
   ASTContext &getASTContext() const { return fn->getASTContext(); }
 };
@@ -2332,8 +2394,8 @@ bool MoveKillsCopyableAddressesChecker::check(SILValue address) {
   // diagnostic.
   bool emittedSingleBBDiagnostic = false;
   for (auto *mvi : useState.markMoves) {
-    emittedSingleBBDiagnostic |=
-        performSingleBasicBlockAnalysis(dataflowState, address, mvi);
+    emittedSingleBBDiagnostic |= performSingleBasicBlockAnalysis(
+        dataflowState, debugInfoBlockSplitPoints, address, mvi);
   }
 
   if (emittedSingleBBDiagnostic) {
@@ -2353,7 +2415,8 @@ bool MoveKillsCopyableAddressesChecker::check(SILValue address) {
   // Ok, we need to perform global dataflow for one of our moves. Initialize our
   // dataflow state engine and then run the dataflow itself.
   dataflowState.init();
-  bool result = dataflowState.process(address, closureConsumes);
+  bool result = dataflowState.process(address, closureConsumes,
+                                      debugInfoBlockSplitPoints);
   return result;
 }
 
@@ -2405,9 +2468,19 @@ class MoveKillsCopyableAddressesCheckerPass : public SILFunctionTransform {
         llvm::makeArrayRef(addressesToCheck.begin(), addressesToCheck.end());
 
     SILOptFunctionBuilder funcBuilder(*this);
+
     MoveKillsCopyableAddressesChecker checker(getFunction(), funcBuilder);
-    bool madeChange = false;
 
+    // If we already had dominance or loop info generated, update them when
+    // splitting blocks.
+    auto *dominanceAnalysis = getAnalysis<DominanceAnalysis>();
+    if (dominanceAnalysis->hasFunctionInfo(fn))
+      checker.setDominanceToUpdate(dominanceAnalysis->get(fn));
+    auto *loopAnalysis = getAnalysis<SILLoopAnalysis>();
+    if (loopAnalysis->hasFunctionInfo(fn))
+      checker.setLoopInfoToUpdate(loopAnalysis->get(fn));
+
+    bool madeChange = false;
     while (!addressToProcess.empty()) {
       auto address = addressToProcess.front();
       addressToProcess = addressToProcess.drop_front(1);
@@ -2419,6 +2492,13 @@ class MoveKillsCopyableAddressesCheckerPass : public SILFunctionTransform {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
 
+    // We update debug info/loop info here, so we just invalidate instructions.
+    if (checker.splitBlocksAfterDebugInfoCarryingInst(fn->getModule())) {
+      AnalysisPreserver preserveDominance(dominanceAnalysis);
+      AnalysisPreserver preserveLoop(loopAnalysis);
+      invalidateAnalysis(SILAnalysis::InvalidationKind::BranchesAndInstructions);
+    }
+
     // Now go through and clone any apply sites that we need to clone.
     SmallVector<SILValue, 8> newArgs;
     bool rewroteCallee = false;