[CopyPropagation] Removed poison mode.

include/swift/SILOptimizer/Utils/CanonicalOSSALifetime.h

@@ -249,9 +249,6 @@ class CanonicalizeOSSALifetime {
   /// liveness may be pruned during canonicalization.
   bool pruneDebugMode;
 
-  /// If true, then new destroy_value instructions will be poison.
-  bool poisonRefsMode;
-
   /// If true and we are processing a value of move_only type, emit a diagnostic
   /// when-ever we need to insert a copy_value.
   std::function<void(Operand *)> moveOnlyCopyValueNotification;
@@ -348,12 +345,11 @@ class CanonicalizeOSSALifetime {
   }
 
   CanonicalizeOSSALifetime(
-      bool pruneDebugMode, bool poisonRefsMode,
-      NonLocalAccessBlockAnalysis *accessBlockAnalysis, DominanceInfo *domTree,
-      InstructionDeleter &deleter,
+      bool pruneDebugMode, NonLocalAccessBlockAnalysis *accessBlockAnalysis,
+      DominanceInfo *domTree, InstructionDeleter &deleter,
       std::function<void(Operand *)> moveOnlyCopyValueNotification = nullptr,
       std::function<void(Operand *)> moveOnlyFinalConsumingUse = nullptr)
-      : pruneDebugMode(pruneDebugMode), poisonRefsMode(poisonRefsMode),
+      : pruneDebugMode(pruneDebugMode),
         moveOnlyCopyValueNotification(moveOnlyCopyValueNotification),
         moveOnlyFinalConsumingUse(moveOnlyFinalConsumingUse),
         accessBlockAnalysis(accessBlockAnalysis), domTree(domTree),
@@ -414,11 +410,9 @@ class CanonicalizeOSSALifetime {
   void findOrInsertDestroys();
 
   void findOrInsertDestroyOnCFGEdge(SILBasicBlock *predBB,
-                                    SILBasicBlock *succBB, bool needsPoison);
+                                    SILBasicBlock *succBB);
 
   void rewriteCopies();
-
-  void injectPoison();
 };
 
 } // end namespace swift

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

@@ -746,8 +746,7 @@ struct MoveOnlyChecker {
           instToDelete->eraseFromParent();
         })),
         canonicalizer(
-            false /*pruneDebugMode*/, false /*poisonRefsMode*/,
-            accessBlockAnalysis, domTree, deleter,
+            false /*pruneDebugMode*/, accessBlockAnalysis, domTree, deleter,
             [&](Operand *use) { consumingUsesNeedingCopy.push_back(use); },
             [&](Operand *use) { finalConsumingUses.push_back(use); }) {}
 

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

@@ -401,9 +401,8 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
   };
 
   CanonicalizeOSSALifetime canonicalizer(
-      false /*pruneDebugMode*/, false /*poisonRefsMode*/, accessBlockAnalysis,
-      domTree, deleter, foundConsumingUseNeedingCopy,
-      foundConsumingUseNotNeedingCopy);
+      false /*pruneDebugMode*/, accessBlockAnalysis, domTree, deleter,
+      foundConsumingUseNeedingCopy, foundConsumingUseNotNeedingCopy);
   auto moveIntroducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
                                             moveIntroducersToProcess.end());
   SmallPtrSet<MarkMustCheckInst *, 4> valuesWithDiagnostics;

lib/SILOptimizer/SILCombiner/SILCombine.cpp

@@ -350,8 +350,8 @@ void SILCombiner::canonicalizeOSSALifetimes(SILInstruction *currentInst) {
   InstructionDeleter deleter(std::move(canonicalizeCallbacks));
 
   DominanceInfo *domTree = DA->get(&Builder.getFunction());
-  CanonicalizeOSSALifetime canonicalizer(
-      false /*prune debug*/, false /*poison refs*/, NLABA, domTree, deleter);
+  CanonicalizeOSSALifetime canonicalizer(false /*prune debug*/, NLABA, domTree,
+                                         deleter);
   CanonicalizeBorrowScope borrowCanonicalizer(deleter);
 
   while (!defsToCanonicalize.empty()) {

lib/SILOptimizer/Transforms/CopyPropagation.cpp

@@ -380,14 +380,12 @@ class CopyPropagation : public SILFunctionTransform {
   /// If true, then borrow scopes will be canonicalized, allowing copies of
   /// guaranteed values to be optimized. Does *not* shrink the borrow scope.
   bool canonicalizeBorrows;
-  /// If true, then new destroy_value instructions will be poison.
-  bool poisonRefs;
 
 public:
   CopyPropagation(bool pruneDebug, bool canonicalizeAll,
-                  bool canonicalizeBorrows, bool poisonRefs)
+                  bool canonicalizeBorrows)
       : pruneDebug(pruneDebug), canonicalizeAll(canonicalizeAll),
-        canonicalizeBorrows(canonicalizeBorrows), poisonRefs(poisonRefs) {}
+        canonicalizeBorrows(canonicalizeBorrows) {}
 
   /// The entry point to this function transformation.
   void run() override;
@@ -439,8 +437,8 @@ void CopyPropagation::run() {
 
   // canonicalizer performs all modifications through deleter's callbacks, so we
   // don't need to explicitly check for changes.
-  CanonicalizeOSSALifetime canonicalizer(pruneDebug, poisonRefs,
-                                         accessBlockAnalysis, domTree, deleter);
+  CanonicalizeOSSALifetime canonicalizer(pruneDebug, accessBlockAnalysis,
+                                         domTree, deleter);
 
   // NOTE: We assume that the function is in reverse post order so visiting the
   //       blocks and pushing begin_borrows as we see them and then popping them
@@ -583,12 +581,10 @@ void CopyPropagation::run() {
 // because it may negatively affect the debugging experience.
 SILTransform *swift::createMandatoryCopyPropagation() {
   return new CopyPropagation(/*pruneDebug*/ true, /*canonicalizeAll*/ true,
-                             /*canonicalizeBorrows*/ false,
-                             /*poisonRefs*/ true);
+                             /*canonicalizeBorrows*/ false);
 }
 
 SILTransform *swift::createCopyPropagation() {
   return new CopyPropagation(/*pruneDebug*/ true, /*canonicalizeAll*/ true,
-                             /*canonicalizeBorrows*/ EnableRewriteBorrows,
-                             /*poisonRefs*/ false);
+                             /*canonicalizeBorrows*/ EnableRewriteBorrows);
 }

lib/SILOptimizer/Utils/CanonicalOSSALifetime.cpp

@@ -48,11 +48,6 @@
 /// alternative would be to model all end_access instructions as deinit
 /// barriers, but that may significantly limit optimization.
 ///
-/// 2. A poison mode supports debugging by setting the poisonRefs flag on any
-/// destroys that have been hoisted. This forces the shadow variables to be
-/// overwritten with a sentinel, preventing the debugger from seeing
-/// deinitialized objects.
-///
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "copy-propagation"
@@ -441,18 +436,16 @@ static DestroyValueInst *findDestroyOnCFGEdge(SILBasicBlock *edgeBB,
 /// canonicalizing other values.  This is especially important when
 /// canonicalization is called within an iterative worklist such as SILCombine.
 void CanonicalizeOSSALifetime::findOrInsertDestroyOnCFGEdge(
-    SILBasicBlock *predBB, SILBasicBlock *succBB, bool needsPoison) {
+    SILBasicBlock *predBB, SILBasicBlock *succBB) {
 
   assert(succBB->getSinglePredecessorBlock() == predBB
          && "value is live-out on another predBB successor: critical edge?");
   auto *di = findDestroyOnCFGEdge(succBB, currentDef);
   if (!di) {
     auto pos = succBB->begin();
-    // TODO: to improve debugability, consider advancing the poison position
-    // ahead of operations that are known not to access weak references.
     SILBuilderWithScope builder(pos);
     auto loc = RegularLocation::getAutoGeneratedLocation(pos->getLoc());
-    di = builder.createDestroyValue(loc, currentDef, needsPoison);
+    di = builder.createDestroyValue(loc, currentDef);
     getCallbacks().createdNewInst(di);
   }
   consumes.recordFinalConsume(di);
@@ -466,7 +459,6 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyOnCFGEdge(
 /// Create a final destroy, immediately after `pos`.
 static void insertDestroyAtInst(SILBasicBlock::iterator pos,
                                 DestroyValueInst *existingDestroy, SILValue def,
-                                bool needsPoison,
                                 CanonicalOSSAConsumeInfo &consumes,
                                 InstModCallbacks &callbacks) {
   if (existingDestroy) {
@@ -476,15 +468,11 @@ static void insertDestroyAtInst(SILBasicBlock::iterator pos,
       }
     }
     consumes.recordFinalConsume(existingDestroy);
-    // Avoid resetting poison just in case this runs twice.
-    if (needsPoison) {
-      existingDestroy->setPoisonRefs(true);
-    }
     return;
   }
   SILBuilderWithScope builder(pos);
   auto loc = RegularLocation::getAutoGeneratedLocation((*pos).getLoc());
-  auto *di = builder.createDestroyValue(loc, def, needsPoison);
+  auto *di = builder.createDestroyValue(loc, def);
   callbacks.createdNewInst(di);
   consumes.recordFinalConsume(di);
 
@@ -498,15 +486,12 @@ static void insertDestroyAtInst(SILBasicBlock::iterator pos,
 //
 // TODO: This has become quite a hack. Instead, the final liveness boundary
 // should be returned in a data structure along with summary information about
-// each block. Then any special logic for handling existing destroys, debug
-// values, and poison should be applied to that block summary which can provide
-// the input to rewriteCopies.
+// each block. Then any special logic for handling existing destroys and debug
+// values should be applied to that block summary which can provide the input
+// to rewriteCopies.
 void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
   auto *defInst = currentDef->getDefiningInstruction();
   DestroyValueInst *existingDestroy = nullptr;
-  bool existingDestroyNeedsPoison = false;
-  // needsPoison is true as soon as an existingDestroy is seen, but sticky.
-  bool needsPoison = false;
   auto instIter = bb->getTerminator()->getIterator();
   while (true) {
     auto *inst = &*instIter;
@@ -524,36 +509,16 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
       // Insert a destroy after this non-consuming use.
       if (isa<TermInst>(inst)) {
         for (auto &succ : bb->getSuccessors()) {
-          findOrInsertDestroyOnCFGEdge(bb, succ, poisonRefsMode);
+          findOrInsertDestroyOnCFGEdge(bb, succ);
         }
       } else {
-        if (existingDestroy) {
-          needsPoison = existingDestroyNeedsPoison;
-        }
         insertDestroyAtInst(std::next(instIter), existingDestroy, currentDef,
-                            needsPoison, consumes, getCallbacks());
+                            consumes, getCallbacks());
       }
       return;
     case PrunedLiveness::LifetimeEndingUse:
-      if (!needsPoison) {
-        // This use becomes a final consume.
-        consumes.recordFinalConsume(inst);
-        return;
-      }
-      // If a destroy needs poison, simply make it so.
-      auto *destroy = dyn_cast<DestroyValueInst>(inst);
-      // Reuse an existing one if we need to.
-      if (!destroy) {
-        destroy = existingDestroy;
-        needsPoison = existingDestroyNeedsPoison;
-      }
-      if (destroy) {
-        destroy->setPoisonRefs(needsPoison);
-        consumes.recordFinalConsume(destroy);
-        return;
-      }
-      // Put this in the set of final consumes that need poison injection.
-      consumes.recordNeedsPoison(inst);
+      // This use becomes a final consume.
+      consumes.recordFinalConsume(inst);
       return;
     }
     // This is not a potential last user. Keep scanning.
@@ -568,33 +533,21 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
             destroy->getOperand());
         if (destroyDef == currentDef) {
           existingDestroy = destroy;
-          // If this destroy is reused, it is not poisoned.
-          existingDestroyNeedsPoison = needsPoison;
-          if (poisonRefsMode) {
-            // Any earlier consume will be poisoned.
-            needsPoison = true;
-          }
         }
       }
     }
     if (instIter == bb->begin()) {
       assert(cast<SILArgument>(currentDef)->getParent() == bb);
-      if (existingDestroy) {
-        needsPoison = existingDestroyNeedsPoison;
-      }
-      insertDestroyAtInst(instIter, existingDestroy, currentDef, needsPoison,
-                          consumes, getCallbacks());
+      insertDestroyAtInst(instIter, existingDestroy, currentDef, consumes,
+                          getCallbacks());
       return;
     }
     --instIter;
     // If the original def is reached, this is a dead live range. Insert a
     // destroy immediately after the def.
     if (&*instIter == defInst) {
-      if (existingDestroy) {
-        needsPoison = existingDestroyNeedsPoison;
-      }
       insertDestroyAtInst(std::next(instIter), existingDestroy, currentDef,
-                          needsPoison, consumes, getCallbacks());
+                          consumes, getCallbacks());
       return;
     }
   }
@@ -634,11 +587,8 @@ void CanonicalizeOSSALifetime::findOrInsertDestroys() {
       // Continue searching upward to find the pruned liveness boundary.
       for (auto *predBB : bb->getPredecessorBlocks()) {
         if (liveness.getBlockLiveness(predBB) == PrunedLiveBlocks::LiveOut) {
-          findOrInsertDestroyOnCFGEdge(predBB, bb, poisonRefsMode);
+          findOrInsertDestroyOnCFGEdge(predBB, bb);
         } else {
-          if (poisonRefsMode) {
-            remnantLiveOutBlocks.insert(predBB);
-          }
           blockWorklist.insert(predBB);
         }
       }
@@ -679,13 +629,6 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
         LLVM_DEBUG(llvm::dbgs() << "  Removing " << *destroy);
         ++NumDestroysEliminated;
       }
-      // If another destroy is reachable on this path, then poison this
-      // destroy. It will already be poisoned if it was inserted on a CFG edge
-      // or another destroy occurs later in the same block.
-      if (!destroy->poisonRefs()
-          && remnantLiveOutBlocks.count(destroy->getParent())) {
-        destroy->setPoisonRefs(true);
-      }
       return true;
     }
 
@@ -702,20 +645,6 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
       return false;
     }
 
-    // Final consumes that need poison, also need a copy.
-    if (consumes.needsPoison(user)) {
-      maybeNotifyMoveOnlyCopy(use);
-      return false;
-    }
-
-    // If another destroy is reachable on this path, then this consume needs
-    // poison even though findOrInsertDestroyInBlock didn't know that.
-    if (remnantLiveOutBlocks.count(user->getParent())) {
-      consumes.recordNeedsPoison(user);
-      maybeNotifyMoveOnlyCopy(use);
-      return false;
-    }
-
     // Ok, this is a final user that isn't a destroy_value. Notify our caller if
     // we were asked to.
     //
@@ -762,10 +691,6 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
   }
   assert(!consumes.hasUnclaimedConsumes());
 
-  // Insert destroys wherever poison is needed. This may recover more debug
-  // values, erasing them from the debugValues set.
-  injectPoison();
-
   // Add any debug_values from Dead blocks into the debugAfterConsume set.
   for (auto *dvi : debugValues) {
     if (liveness.getBlockLiveness(dvi->getParent()) == PrunedLiveBlocks::Dead) {
@@ -785,54 +710,10 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
   }
 }
 
-/// Insert poison destroys after each consume that needs poison.
-void CanonicalizeOSSALifetime::injectPoison() {
-  for (SILInstruction *consume : consumes.getNeedsPoisonConsumes()) {
-    auto createDestroy = [&](SILBuilder &builder) {
-      auto loc = RegularLocation::getAutoGeneratedLocation(
-        builder.getInsertionPoint()->getLoc());
-      auto *di = builder.createDestroyValue(loc, currentDef,
-                                            /*needsPoison*/ true);
-      getCallbacks().createdNewInst(di);
-      ++NumDestroysGenerated;
-      LLVM_DEBUG(llvm::dbgs() << "  Destroy at last use " << *di);
-    };
-    if (auto *branch = dyn_cast<BranchInst>(consume)) {
-      // At a phi, destroy the value before branching.
-      //
-      // A copy must have been created for the branch operand during
-      // rewriteCopies (but there's no easy way to assert that here).
-      SILBuilderWithScope builder(branch);
-      createDestroy(builder);
-    } else {
-      // For normal instructions and non-phi block terminators, destroy the
-      // value after the operation.
-      SILBuilderWithScope::insertAfter(consume, createDestroy);
-    }
-  }
-}
-
 //===----------------------------------------------------------------------===//
 //                            MARK: Top-Level API
 //===----------------------------------------------------------------------===//
 
-/// True if \p def should be canonicalized in poisonRefs mode.
-///
-/// Currently, we only handle owned values because, for now, the goal is to
-/// shorten lifetimes to mimic -O behavior, not to remove copies.
-///
-/// IRGen must also have support for injecting poison into values of this type.
-static bool shouldCanonicalizeWithPoison(SILValue def) {
-  assert(def->getType().isObject() && "only SIL values are canonicalized");
-  auto objectTy = def->getType().unwrapOptionalType();
-
-  // TODO: Handle structs, enums, and tuples.
-  //
-  // TODO: Functions are not currently handled because of closure lifetime
-  // bugs. Noescape closures don't always have a dependence.
-  return objectTy.isAnyClassReferenceType();
-}
-
 /// Canonicalize a single extended owned lifetime.
 bool CanonicalizeOSSALifetime::canonicalizeValueLifetime(SILValue def) {
   if (def->getOwnershipKind() != OwnershipKind::Owned)
@@ -841,9 +722,6 @@ bool CanonicalizeOSSALifetime::canonicalizeValueLifetime(SILValue def) {
   if (def->isLexical())
     return false;
 
-  if (poisonRefsMode && !shouldCanonicalizeWithPoison(def))
-    return false;
-
   LLVM_DEBUG(llvm::dbgs() << "  Canonicalizing: " << def);
 
   // Note: There is no need to register callbacks with this utility. 'onDelete'