Merge pull request #41718 from gottesmm/pr-2fa50bd880db04a10e5bbc6885b69db9dcc3f777

[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

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;
 }