[move-function] Rather than breaking block at the SIL level after moved dbg_value, do it late as an LLVM pass on llvm.dbg.addr

This has a few nice benefits:

1. The splitting happens after LLVM optimizations have run. This ensures that
LLVM will not join these blocks no matter what! The author of this commit has
found that in certain cases LLVM does this even at -Onone. By running this late,
we get the benefit we are looking for: working around the bad SelectionDAG
behavior.

2. This block splitting is just a workaround for the above mentioned unfortunate
SelectionDAG behavior. By doing this when we remove the workaround, we will not
have to update SIL level tests... instead we will just remove a small LLVM pass.

Some additional notes:

1. Only moved values will ever have llvm.dbg.addr emitted today, so we do not
have to worry about this impacting the rest of the language.

2. The pass's behavior is tested at the IR level by move_function_dbginfo.swift.

Author: gottesmm

include/swift/LLVMPasses/Passes.h

@@ -102,7 +102,6 @@ namespace swift {
     static char ID;
     InlineTreePrinter() : llvm::ModulePass(ID) {}
   };
-
 } // end namespace swift
 
 #endif

include/swift/LLVMPasses/PassesFwd.h

@@ -25,6 +25,7 @@ namespace llvm {
   void initializeSwiftARCContractPass(PassRegistry &);
   void initializeInlineTreePrinterPass(PassRegistry &);
   void initializeSwiftMergeFunctionsPass(PassRegistry &);
+  void initializeSwiftDbgAddrBlockSplitterPass(PassRegistry &);
 }
 
 namespace swift {
@@ -33,6 +34,7 @@ namespace swift {
   llvm::ModulePass *createInlineTreePrinterPass();
   llvm::ModulePass *createSwiftMergeFunctionsPass(bool ptrAuthEnabled,
                                                   unsigned ptrAuthKey);
+  llvm::FunctionPass *createSwiftDbgAddrBlockSplitter();
   llvm::ImmutablePass *createSwiftAAWrapperPass();
   llvm::ImmutablePass *createSwiftRCIdentityPass();
 } // end namespace swift

lib/IRGen/IRGen.cpp

@@ -146,6 +146,11 @@ static void addThreadSanitizerPass(const PassManagerBuilder &Builder,
   PM.add(createThreadSanitizerLegacyPassPass());
 }
 
+static void addSwiftDbgAddrBlockSplitterPass(const PassManagerBuilder &Builder,
+                                             legacy::PassManagerBase &PM) {
+  PM.add(createSwiftDbgAddrBlockSplitter());
+}
+
 static void addSanitizerCoveragePass(const PassManagerBuilder &Builder,
                                      legacy::PassManagerBase &PM) {
   const PassManagerBuilderWrapper &BuilderWrapper =
@@ -289,6 +294,13 @@ void swift::performLLVMOptimizations(const IRGenOptions &Opts,
       });
   }
 
+  if (RunSwiftSpecificLLVMOptzns) {
+    PMBuilder.addExtension(PassManagerBuilder::EP_OptimizerLast,
+                           addSwiftDbgAddrBlockSplitterPass);
+    PMBuilder.addExtension(PassManagerBuilder::EP_EnabledOnOptLevel0,
+                           addSwiftDbgAddrBlockSplitterPass);
+  }
+
   // Configure the function passes.
   legacy::FunctionPassManager FunctionPasses(Module);
   FunctionPasses.add(createTargetTransformInfoWrapperPass(

lib/LLVMPasses/CMakeLists.txt

@@ -6,6 +6,7 @@ add_swift_host_library(swiftLLVMPasses STATIC
   LLVMARCContract.cpp
   LLVMInlineTree.cpp
   LLVMMergeFunctions.cpp
+  DbgAddrBlockSplitter.cpp
 
   LLVM_LINK_COMPONENTS
   analysis

lib/LLVMPasses/DbgAddrBlockSplitter.cpp

@@ -0,0 +1,79 @@
+//===--- DbgAddrBlockSplitter.cpp -----------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// This simple pass runs late after all LLVM level optimization passes have
+/// completed and break blocks at llvm.dbg.addr when we are compiling at
+/// -Onone. This helps us avoid bad behavior in SelectionDAG where SelectionDAG
+/// in certain cases sink llvm.dbg.addr to the end of blocks.
+///
+//===----------------------------------------------------------------------===//
+
+#include "swift/LLVMPasses/Passes.h"
+#include "swift/LLVMPasses/PassesFwd.h"
+#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/Pass.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
+
+using namespace llvm;
+
+namespace {
+
+struct SwiftDbgAddrBlockSplitter : FunctionPass {
+  static char ID;
+  SwiftDbgAddrBlockSplitter() : FunctionPass(ID) {}
+
+  bool runOnFunction(Function &fn) override;
+};
+
+} // namespace
+
+bool SwiftDbgAddrBlockSplitter::runOnFunction(Function &fn) {
+  SmallVector<Instruction *, 32> breakBlockPoints;
+
+  for (auto &block : fn) {
+    for (auto &inst : block) {
+      if (isa<DbgAddrIntrinsic>(&inst)) {
+        breakBlockPoints.push_back(&*std::next(inst.getIterator()));
+      }
+    }
+  }
+
+  if (breakBlockPoints.empty())
+    return false;
+
+  bool madeChange = false;
+
+  while (!breakBlockPoints.empty()) {
+    auto *i = breakBlockPoints.pop_back_val();
+    if (i->isTerminator())
+      continue;
+    SplitBlock(i->getParent(), i);
+    madeChange = true;
+  }
+
+  return madeChange;
+}
+
+char SwiftDbgAddrBlockSplitter::ID = 0;
+INITIALIZE_PASS_BEGIN(SwiftDbgAddrBlockSplitter,
+                      "swift-dbg-addr-block-splitter",
+                      "Swift pass that splits blocks after llvm.dbg.addr",
+                      false, false)
+INITIALIZE_PASS_END(SwiftDbgAddrBlockSplitter, "swift-dbg-addr-block-splitter",
+                    "Swift pass that splits blocks after llvm.dbg.addr", false,
+                    false)
+
+llvm::FunctionPass *swift::createSwiftDbgAddrBlockSplitter() {
+  initializeSwiftDbgAddrBlockSplitterPass(
+      *llvm::PassRegistry::getPassRegistry());
+  return new SwiftDbgAddrBlockSplitter();
+}

lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp

@@ -1398,19 +1398,17 @@ struct DataflowState {
         applySiteToPromotedArgIndices(applySiteToPromotedArgIndices),
         closureConsumes(closureConsumes) {}
   void init();
-  bool
-  process(SILValue address,
-          SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
-          SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints);
+  bool process(
+      SILValue address,
+      SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
   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,
-      SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints);
+      SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
   void clear() {
     useBlocks.clear();
     initBlocks.clear();
@@ -1429,8 +1427,7 @@ bool DataflowState::cleanupAllDestroyAddr(
     SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
     BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
     BasicBlockSet &blocksWithMovesThatAreNowTakes,
-    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
-    SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints) {
+    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
   bool madeChange = false;
   BasicBlockWorklist worklist(fn);
 
@@ -1544,12 +1541,8 @@ bool DataflowState::cleanupAllDestroyAddr(
       if (auto varInfo = debugVarInst.getVarInfo()) {
         SILBuilderWithScope reinitBuilder(*reinit);
         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);
+        reinitBuilder.createDebugValue(debugVarInst.inst->getLoc(), address,
+                                       *varInfo, false, /*was moved*/ true);
       }
     }
     madeChange = true;
@@ -1591,8 +1584,7 @@ bool DataflowState::cleanupAllDestroyAddr(
 
 bool DataflowState::process(
     SILValue address,
-    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers,
-    SmallSetVector<SILInstruction *, 8> &debugInfoBlockSplitPoints) {
+    SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
   SILFunction *fn = address->getFunction();
   assert(fn);
 
@@ -1811,25 +1803,13 @@ 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, debugInfoBlockSplitPoints);
+      postDominatingConsumingUsers);
 
   return madeChange;
 }
@@ -1932,19 +1912,6 @@ 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(),
@@ -1953,12 +1920,6 @@ 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,
@@ -1970,20 +1931,6 @@ 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;
-  }
-
   bool performSingleBasicBlockAnalysis(SILValue address,
                                        MarkUnresolvedMoveAddrInst *mvi);
 
@@ -2131,7 +2078,6 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
     if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
       if (auto varInfo = debug.getVarInfo()) {
         SILBuilderWithScope undefBuilder(builder);
-        debugInfoBlockSplitPoints.insert(*debug);
         undefBuilder.setCurrentDebugScope(debug->getDebugScope());
         undefBuilder.createDebugValue(
             debug->getLoc(),
@@ -2242,7 +2188,6 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
     if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
       if (auto varInfo = debug.getVarInfo()) {
         {
-          debugInfoBlockSplitPoints.insert(*debug);
           SILBuilderWithScope undefBuilder(builder);
           undefBuilder.setCurrentDebugScope(debug->getDebugScope());
           undefBuilder.createDebugValue(
@@ -2257,10 +2202,9 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
           auto *next = interestingUser->getNextInstruction();
           SILBuilderWithScope reinitBuilder(next);
           reinitBuilder.setCurrentDebugScope(debug->getDebugScope());
-          auto *dvi = reinitBuilder.createDebugValue(debug->getLoc(),
-                                                     address, *varInfo, false,
-                                                     /*was moved*/ true);
-          debugInfoBlockSplitPoints.insert(dvi);
+          reinitBuilder.createDebugValue(debug->getLoc(), address, *varInfo,
+                                         false,
+                                         /*was moved*/ true);
         }
       }
       debug.markAsMoved();
@@ -2296,7 +2240,6 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
                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->getDebugScope());
         undefBuilder.createDebugValue(
@@ -2415,8 +2358,7 @@ 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,
-                                      debugInfoBlockSplitPoints);
+  bool result = dataflowState.process(address, closureConsumes);
   return result;
 }
 
@@ -2471,15 +2413,6 @@ class MoveKillsCopyableAddressesCheckerPass : public SILFunctionTransform {
 
     MoveKillsCopyableAddressesChecker checker(getFunction(), funcBuilder);
 
-    // 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();
@@ -2492,13 +2425,6 @@ 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;

lib/SILOptimizer/Mandatory/MoveKillsCopyableValuesChecker.cpp

@@ -380,7 +380,6 @@ bool MoveKillsCopyableValuesChecker::check() {
   //
   // TODO: We should add llvm.dbg.addr support for fastisel and also teach
   // CodeGen how to handle llvm.dbg.addr better.
-  SmallVector<SILInstruction *, 8> successMovesDbgInfoCarryingInsts;
   bool emittedDiagnostic = false;
   while (!valuesToProcess.empty()) {
     auto lexicalValue = valuesToProcess.front();
@@ -421,7 +420,6 @@ bool MoveKillsCopyableValuesChecker::check() {
         } else {
           LLVM_DEBUG(llvm::dbgs() << "    WithinBoundary: No!\n");
           if (auto varInfo = dbgVarInst.getVarInfo()) {
-            successMovesDbgInfoCarryingInsts.push_back(*dbgVarInst);
             auto *next = mvi->getNextInstruction();
             SILBuilderWithScope builder(next);
             // We need to make sure any undefs we put in are the same loc/debug
@@ -445,24 +443,6 @@ bool MoveKillsCopyableValuesChecker::check() {
     }
   }
 
-  // If we emitted any diagnostics, we are going to fail and thus don't need to
-  // use any compile time to break blocks since a user will never debug such
-  // programs.
-  if (emittedDiagnostic)
-    return false;
-
-  // Ok! Now break before the instruction after our debug info generating inst
-  // so that the SelectionDAG behavior mentioned above on the declaration of
-  // successMovesDbgInfoCarryingInst.
-  if (!successMovesDbgInfoCarryingInsts.empty()) {
-    SILBuilderContext ctx(mod);
-    do {
-      auto *next = successMovesDbgInfoCarryingInsts.pop_back_val();
-      splitBasicBlockAndBranch(ctx, next->getNextInstruction(),
-                               dominanceToUpdate, loopInfoToUpdate);
-    } while (!successMovesDbgInfoCarryingInsts.empty());
-  }
-
   return false;
 }