Merge pull request #72964 from Snowy1803/mem2reg-salvage-store

[DebugInfo] Salvage all stores

Author: Snowy1803

include/swift/SIL/SILBuilder.h

@@ -262,7 +262,11 @@ class SILBuilder {
   /// scopes. To avoid a verification error later in the pipeline, drop all
   /// variables without a proper source location.
   bool shouldDropVariable(SILDebugVariable Var, SILLocation Loc) {
-    return !Var.ArgNo && Loc.isSynthesizedAST();
+    if (Var.ArgNo)
+      return false;
+    if (Var.Loc)
+      return Var.Loc->isSynthesizedAST();
+    return Loc.isSynthesizedAST();
   }
 
 

include/swift/SIL/SILLocation.h

@@ -391,6 +391,16 @@ class SILLocation {
   /// Check is this location is part of a function's implicit prologue.
   bool isInPrologue() const { return kindAndFlags.fields.inPrologue; }
 
+  /// Returns this location with the auto-generated and prologue bits stripped.
+  /// These bits only make sense for instructions, and should be stripped for
+  /// variables.
+  SILLocation strippedForDebugVariable() const {
+    SILLocation loc = *this;
+    loc.kindAndFlags.fields.autoGenerated = false;
+    loc.kindAndFlags.fields.inPrologue = false;
+    return loc;
+  }
+
   /// Check if the corresponding source code location definitely points
   ///  to the end of the AST node.
   bool pointsToEnd() const;

include/swift/SILOptimizer/Utils/DebugOptUtils.h

@@ -48,6 +48,14 @@ inline void deleteAllDebugUses(SILInstruction *inst,
 /// new `debug_value` instruction before \p I is deleted.
 void salvageDebugInfo(SILInstruction *I);
 
+/// Transfer debug info associated with the store-like instruction \p SI to a
+/// new `debug_value` instruction before \p SI is deleted.
+/// \param SI The store instruction being deleted
+/// \param SrcVal The old source, where the debuginfo should be propagated to
+/// \param DestVal The old destination, where the debuginfo was
+void salvageStoreDebugInfo(SILInstruction *SI,
+                           SILValue SrcVal, SILValue DestVal);
+
 /// Transfer debug information associated with the result of \p load to the
 /// load's address operand.
 ///

lib/IRGen/IRGenDebugInfo.cpp

@@ -3186,10 +3186,14 @@ void IRGenDebugInfoImpl::emitVariableDeclaration(
   // Create the descriptor for the variable.
   unsigned DVarLine = DInstLine;
   uint16_t DVarCol = DInstLoc.Column;
-  if (VarInfo.Loc) {
-    auto DVarLoc = getStartLocation(VarInfo.Loc);
-    DVarLine = DVarLoc.Line;
-    DVarCol = DVarLoc.Column;
+  auto VarInfoLoc = VarInfo.Loc ? VarInfo.Loc : DbgInstLoc;
+  if (VarInfoLoc) {
+    auto VarLoc = VarInfoLoc->strippedForDebugVariable();
+    if (VarLoc != DbgInstLoc) {
+      auto DVarLoc = getStartLocation(VarLoc);
+      DVarLine = DVarLoc.Line;
+      DVarCol = DVarLoc.Column;
+    }
   }
   llvm::DIScope *VarScope = Scope;
   if (ArgNo == 0 && VarInfo.Scope) {

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -756,8 +756,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         // an alloc_stack), which don't have any `op_deref` in its
         // di-expression, because that memory doesn't need to be initialized
         // when `debug_value` is referencing it.
-        if (cast<DebugValueInst>(&I)->hasAddrVal() &&
-            cast<DebugValueInst>(&I)->exprStartsWithDeref())
+        if (!DebugValueInst::hasAddrVal(&I))
           requireBitsSet(bits, I.getOperand(0), &I);
         break;
       case SILInstructionKind::UncheckedTakeEnumDataAddrInst: {

lib/SILOptimizer/Transforms/CopyForwarding.cpp

@@ -69,6 +69,7 @@
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/CFGOptUtils.h"
+#include "swift/SILOptimizer/Utils/DebugOptUtils.h"
 #include "swift/SILOptimizer/Utils/ValueLifetime.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/Statistic.h"
@@ -698,6 +699,8 @@ propagateCopy(CopyAddrInst *CopyInst) {
       SILBuilderWithScope(CurrentCopy)
           .createDestroyAddr(CurrentCopy->getLoc(), CurrentCopy->getDest());
     }
+    swift::salvageStoreDebugInfo(CurrentCopy, CurrentCopy->getSrc(),
+                                 CurrentCopy->getDest());
     CurrentCopy->eraseFromParent();
     HasChanged = true;
     ++NumCopyForward;
@@ -706,6 +709,8 @@ propagateCopy(CopyAddrInst *CopyInst) {
   // Forward propagation failed. Attempt to backward propagate.
   if (CurrentCopy->isInitializationOfDest() && backwardPropagateCopy()) {
     LLVM_DEBUG(llvm::dbgs() << "  Reversing Copy:" << *CurrentCopy);
+    swift::salvageStoreDebugInfo(CurrentCopy, CurrentCopy->getDest(),
+                                 CurrentCopy->getSrc());
     CurrentCopy->eraseFromParent();
     HasChanged = true;
     ++NumCopyBackward;
@@ -817,6 +822,9 @@ forwardDeadTempCopy(CopyAddrInst *destCopy) {
       .createDestroyAddr(srcCopy->getLoc(), srcCopy->getDest());
   }
 
+  // Salvage debug values before deleting them.
+  swift::salvageStoreDebugInfo(srcCopy, srcCopy->getSrc(), srcCopy->getDest());
+
   // Delete all dead debug_value instructions
   for (auto *deadDebugUser : debugInstsToDelete) {
     deadDebugUser->eraseFromParent();
@@ -1190,9 +1198,13 @@ bool CopyForwarding::backwardPropagateCopy() {
   // Now that an init was found, it is safe to substitute all recorded uses
   // with the copy's dest.
   for (auto *Oper : ValueUses) {
-    Oper->set(CurrentCopy->getDest());
-    if (isa<CopyAddrInst>(Oper->getUser()))
+    if (auto *SI = dyn_cast<CopyAddrInst>(Oper->getUser())) {
       HasForwardedToCopy = true;
+      // This instruction gets "replaced", so we need to salvage its previous
+      // debug info.
+      swift::salvageStoreDebugInfo(SI, SI->getSrc(), SI->getDest());
+    }
+    Oper->set(CurrentCopy->getDest());
   }
   return true;
 }

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -301,68 +301,6 @@ replaceDestroy(DestroyAddrInst *dai, SILValue newValue, SILBuilderContext &ctx,
   prepareForDeletion(dai, instructionsToDelete);
 }
 
-/// Whether the specified debug_value's operand names the address at the
-/// indicated alloc_stack.
-///
-/// If it's a guaranteed alloc_stack (i.e. a store_borrow location), that
-/// includes the values produced by any store_borrows whose destinations are the
-/// alloc_stack since those values amount to aliases for the alloc_stack's
-/// storage.
-static bool isDebugValueOfAllocStack(DebugValueInst *dvi, AllocStackInst *asi) {
-  auto value = dvi->getOperand();
-  if (value == asi)
-    return true;
-  auto *sbi = dyn_cast<StoreBorrowInst>(value);
-  if (!sbi)
-    return false;
-  return sbi->getDest() == asi;
-}
-
-/// Promote a DebugValue w/ address value to a DebugValue of non-address value.
-static void promoteDebugValueAddr(DebugValueInst *dvai, SILValue value,
-                                  SILBuilderContext &ctx,
-                                  InstructionDeleter &deleter) {
-  assert(dvai->getOperand()->getType().isLoadable(*dvai->getFunction()) &&
-         "Unexpected promotion of address-only type!");
-  assert(value && "Expected valid value");
-
-  // Avoid inserting the same debug_value twice.
-  //
-  // We remove the di expression when comparing since:
-  //
-  // 1. dvai is on will always have the deref diexpr since it is on addresses.
-  //
-  // 2. We are only trying to delete debug_var that are on values... values will
-  //    never have an op_deref meaning that the comparison will always fail and
-  //    not serve out purpose here.
-  auto dvaiWithoutDIExpr = dvai->getVarInfo()->withoutDIExpr();
-  for (auto *use : value->getUses()) {
-    if (auto *dvi = dyn_cast<DebugValueInst>(use->getUser())) {
-      if (!dvi->hasAddrVal() && *dvi->getVarInfo() == dvaiWithoutDIExpr) {
-        deleter.forceDelete(dvai);
-        return;
-      }
-    }
-  }
-
-  // Drop op_deref if dvai is actually a debug_value instruction
-  auto varInfo = *dvai->getVarInfo();
-  if (isa<DebugValueInst>(dvai)) {
-    auto &diExpr = varInfo.DIExpr;
-    // FIXME: There should always be a DIExpr starting with an op_deref here
-    // The debug_value is attached to a pointer type, and those don't exist
-    // in Swift, so they should always be dereferenced.
-    // However, this rule is broken in a lot of spaces, so we have to leave
-    // this check to recover from wrong info
-    if (diExpr && diExpr.startsWithDeref())
-      diExpr.eraseElement(diExpr.element_begin());
-  }
-
-  SILBuilderWithScope b(dvai, ctx);
-  b.createDebugValue(dvai->getLoc(), value, std::move(varInfo));
-  deleter.forceDelete(dvai);
-}
-
 /// Returns true if \p I is a load which loads from \p ASI.
 static bool isLoadFromStack(SILInstruction *i, AllocStackInst *asi) {
   if (!isa<LoadInst>(i) && !isa<LoadBorrowInst>(i))
@@ -1172,14 +1110,8 @@ SILInstruction *StackAllocationPromoter::promoteAllocationInBlock(
       continue;
     }
 
-    // Replace debug_value w/ address value with debug_value of
-    // the promoted value.
-    // if we have a valid value to use at this point. Otherwise we'll
-    // promote this when we deal with hooking up phis.
+    // Debug values will automatically be salvaged, we can ignore them.
     if (auto *dvi = DebugValueInst::hasAddrVal(inst)) {
-      if (isDebugValueOfAllocStack(dvi, asi) && runningVals)
-        promoteDebugValueAddr(dvi, runningVals->value.replacement(asi, dvi),
-                              ctx, deleter);
       continue;
     }
 
@@ -1432,12 +1364,8 @@ void StackAllocationPromoter::fixBranchesAndUses(
     // on.
     SILBasicBlock *userBlock = user->getParent();
 
+    // Debug values will automatically be salvaged, we can ignore them.
     if (auto *dvi = DebugValueInst::hasAddrVal(user)) {
-      // Replace debug_value w/ address-type value with
-      // a new debug_value w/ promoted value.
-      auto def = getEffectiveLiveInValues(phiBlocks, userBlock);
-      promoteDebugValueAddr(dvi, def.replacement(asi, dvi), ctx, deleter);
-      ++NumInstRemoved;
       continue;
     }
 
@@ -2036,20 +1964,8 @@ void MemoryToRegisters::removeSingleBlockAllocation(AllocStackInst *asi) {
       continue;
     }
 
-    // Replace debug_value w/ address value with debug_value of
-    // the promoted value.
+    // Debug values will automatically be salvaged, we can ignore them.
     if (auto *dvi = DebugValueInst::hasAddrVal(inst)) {
-      if (isDebugValueOfAllocStack(dvi, asi)) {
-        if (runningVals) {
-          promoteDebugValueAddr(dvi, runningVals->value.replacement(asi, dvi),
-                                ctx, deleter);
-        } else {
-          // Drop debug_value of uninitialized void values.
-          assert(asi->getElementType().isVoid() &&
-                 "Expected initialization of non-void type!");
-          deleter.forceDelete(dvi);
-        }
-      }
       continue;
     }
 

lib/SILOptimizer/Utils/GenericCloner.cpp

@@ -68,6 +68,8 @@ void GenericCloner::populateCloned() {
   SILBasicBlock *ClonedEntryBB = Cloned->createBasicBlock();
   getBuilder().setInsertionPoint(ClonedEntryBB);
 
+  RemappedScopeCache.insert({Original.getDebugScope(), Cloned->getDebugScope()});
+
   // Create the entry basic block with the function arguments.
   auto origConv = Original.getConventions();
   unsigned ArgIdx = 0;
@@ -138,26 +140,6 @@ void GenericCloner::populateCloned() {
               mappedType, OrigArg->getDecl());
           NewArg->copyFlags(cast<SILFunctionArgument>(OrigArg));
 
-          // Try to create a new debug_value from an existing debug_value w/
-          // address value for the argument. We do this before storing to
-          // ensure that when we are cloning code in ossa the argument has
-          // not been consumed by the store below.
-          for (Operand *ArgUse : OrigArg->getUses()) {
-            if (auto *DVI = DebugValueInst::hasAddrVal(ArgUse->getUser())) {
-              auto *oldScope = getBuilder().getCurrentDebugScope();
-              getBuilder().setCurrentDebugScope(
-                  remapScope(DVI->getDebugScope()));
-              auto VarInfo = DVI->getVarInfo();
-              assert(VarInfo && VarInfo->DIExpr &&
-                     "No DebugVarInfo or no DIExpr operand?");
-              // Drop the op_deref
-              VarInfo->DIExpr.eraseElement(VarInfo->DIExpr.element_begin());
-              getBuilder().createDebugValue(DVI->getLoc(), NewArg, *VarInfo);
-              getBuilder().setCurrentDebugScope(oldScope);
-              break;
-            }
-          }
-
           // Store the new direct parameter to an alloc_stack.
           createAllocStack();
           SILValue addr;

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -1826,6 +1826,44 @@ void swift::endLifetimeAtLeakingBlocks(SILValue value,
       });
 }
 
+/// Create a new debug value from a store and a debug variable.
+static void transferStoreDebugValue(DebugVarCarryingInst DefiningInst,
+                                    SILInstruction *SI,
+                                    SILValue original) {
+  auto VarInfo = DefiningInst.getVarInfo();
+  if (!VarInfo)
+    return;
+  // Transfer the location and scope of the debug value to the debug variable,
+  // unless they are the same, in which case we don't need to store it twice.
+  // That way, the variable will point to its declaration, and the debug_value
+  // will point to the assignment point.
+  if (!VarInfo->Loc && !SI->getLoc().hasSameSourceLocation(DefiningInst->getLoc()))
+    VarInfo->Loc = DefiningInst->getLoc();
+  if (!VarInfo->Scope && SI->getDebugScope() != DefiningInst->getDebugScope())
+    VarInfo->Scope = DefiningInst->getDebugScope();
+  // Fix the op_deref.
+  if (!isa<CopyAddrInst>(SI) && VarInfo->DIExpr.startsWithDeref())
+    VarInfo->DIExpr.eraseElement(VarInfo->DIExpr.element_begin());
+  else if (isa<CopyAddrInst>(SI) && !VarInfo->DIExpr.startsWithDeref())
+    VarInfo->DIExpr.prependElements({
+      SILDIExprElement::createOperator(SILDIExprOperator::Dereference)});
+  // Note: The instruction should logically be in the SI's scope.
+  // However, LLVM does not support variables and stores in different scopes,
+  // so we use the variable's scope.
+  SILBuilder(SI, DefiningInst->getDebugScope())
+    .createDebugValue(SI->getLoc(), original, *VarInfo);
+}
+
+void swift::salvageStoreDebugInfo(SILInstruction *SI,
+                                  SILValue SrcVal, SILValue DestVal) {
+  if (auto *ASI = dyn_cast_or_null<AllocStackInst>(
+          DestVal.getDefiningInstruction())) {
+    transferStoreDebugValue(ASI, SI, SrcVal);
+    for (Operand *U : getDebugUses(ASI))
+      transferStoreDebugValue(U->getUser(), SI, SrcVal);
+  }
+}
+
 // TODO: this currently fails to notify the pass with notifyNewInstruction.
 //
 // TODO: whenever a debug_value is inserted at a new location, check that no
@@ -1837,18 +1875,13 @@ void swift::salvageDebugInfo(SILInstruction *I) {
 
   if (auto *SI = dyn_cast<StoreInst>(I)) {
     if (SILValue DestVal = SI->getDest())
-      if (auto *ASI = dyn_cast_or_null<AllocStackInst>(
-              DestVal.getDefiningInstruction())) {
-        if (auto VarInfo = ASI->getVarInfo()) {
-          // Always propagate destination location for incoming arguments (as
-          // their location must be unique) as well as when store source
-          // location is compiler-generated.
-          bool UseDestLoc = VarInfo->ArgNo || SI->getLoc().isAutoGenerated();
-          SILBuilder(SI, ASI->getDebugScope())
-            .createDebugValue(UseDestLoc ? ASI->getLoc() : SI->getLoc(),
-                              SI->getSrc(), *VarInfo);
-        }
-      }
+      salvageStoreDebugInfo(SI, SI->getSrc(), DestVal);
+  }
+  if (auto *SI = dyn_cast<StoreBorrowInst>(I)) {
+    if (SILValue DestVal = SI->getDest())
+      salvageStoreDebugInfo(SI, SI->getSrc(), DestVal);
+    for (Operand *U : getDebugUses(SI))
+      transferStoreDebugValue(U->getUser(), SI, SI->getSrc());
   }
   // If a `struct` SIL instruction is "unwrapped" and removed,
   // for instance, in favor of using its enclosed value directly,

lib/SILOptimizer/Utils/InstructionDeleter.cpp

@@ -228,6 +228,7 @@ void InstructionDeleter::deleteWithUses(SILInstruction *inst, bool fixLifetimes,
   // Recursively visit all uses while growing toDeleteInsts in def-use order and
   // dropping dead operands.
   SmallVector<SILInstruction *, 4> toDeleteInsts;
+  SmallVector<Operand *, 4> toDropUses;
 
   toDeleteInsts.push_back(inst);
   swift::salvageDebugInfo(inst);
@@ -242,11 +243,14 @@ void InstructionDeleter::deleteWithUses(SILInstruction *inst, bool fixLifetimes,
         assert(!isa<BranchInst>(user) && "can't delete phis");
 
         toDeleteInsts.push_back(user);
+        toDropUses.push_back(use);
         swift::salvageDebugInfo(user);
-        use->drop();
       }
     }
   }
+  // Drop all after salvage debug info has been run.
+  for (auto *use : toDropUses)
+    use->drop();
   // Process the remaining operands. Insert destroys for consuming
   // operands. Track newly dead operand values. Instructions with multiple dead
   // operands may occur in toDeleteInsts multiple times.

test/AutoDiff/compiler_crashers_fixed/58660-conflicting-debug-info-inlining.swift

@@ -47,7 +47,7 @@ struct MyModel: Differentiable {
 // CHECK-LABEL: // pullback of MyModel.member4()
 // CHECK-NOT: debug_value %{{.*}} : $MyModel.TangentVector, var, name %{{.*}}, argno 1, scope
 // CHECK: bb0(%{{.*}} : $_AD__$s4main7MyModelV7member4yyF_bb3__Pred__src_0_wrt_0):
-// CHECK: debug_value %{{.*}} : $MyModel.TangentVector, var, name "derivative of 'self' in scope at {{.*}} (scope #1)", scope
+// CHECK: debug_value %{{.*}} : $MyModel.TangentVector, var, (name "derivative of 'self' in scope at {{.*}} (scope #1)"{{.*}}), scope
     // Must be a differentiable type.
     var localVar: Float = 0