[DebugInfo] [Mem2Reg] Move debug info promotion to salvageDebugInfo

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/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/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,36 @@ void swift::endLifetimeAtLeakingBlocks(SILValue value,
       });
 }
 
+// Replace a store with an appropriate debug value.
+static void transferStoreDebugValue(DebugVarCarryingInst DefiningInst,
+                                    SILInstruction *SI,
+                                    SILValue original) {
+  auto VarInfo = DefiningInst.getVarInfo();
+  if (!VarInfo)
+    return;
+  if (!VarInfo->Loc && !SI->getLoc().hasSameSourceLocation(DefiningInst->getLoc()))
+    VarInfo->Loc = DefiningInst->getLoc();
+  if (!VarInfo->Scope && SI->getDebugScope() != DefiningInst->getDebugScope())
+    VarInfo->Scope = DefiningInst->getDebugScope();
+  if (!isa<CopyAddrInst>(SI) && VarInfo->DIExpr.startsWithDeref())
+    VarInfo->DIExpr.eraseElement(VarInfo->DIExpr.element_begin());
+  // 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 +1867,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,15 @@ 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
 

test/DebugInfo/sroa_debug_value.sil

@@ -1,4 +1,5 @@
 // RUN: %target-sil-opt -enable-sil-verify-all -sil-print-debuginfo -sroa %s | %FileCheck --check-prefix=CHECK-SROA %s
+// RUN: %target-sil-opt -enable-sil-verify-all -sil-print-debuginfo -sroa -mem2reg %s | %FileCheck --check-prefix=CHECK-MEM2REG %s
 sil_stage canonical
 
 import Builtin
@@ -51,5 +52,14 @@ bb0(%0 : $Int64, %1 : $Int64):
   store %1 to %15 : $*Int64, loc "sroa.swift":10:17, scope 2
   // CHECK-SROA: store %1 to %[[ALLOC_Y]]
   dealloc_stack %4 : $*MyStruct, loc "sroa.swift":8:9, scope 2
+  // Make sure function arguments' SSA values are properly connected to both source variables
+  // CHECK-MEM2REG: debug_value %0 : $Int64, var, (name "my_struct", loc "sroa.swift":8:9
+  // CHECK-MEM2REG-SAME: type $*MyStruct, expr op_fragment:#MyStruct.x
+  // CHECK-MEM2REG: debug_value %0 : $Int64, let, (name "my_copy", loc "sroa.swift":7:10
+  // CHECK-MEM2REG-SAME: type $MyStruct, expr op_fragment:#MyStruct.x
+  // CHECK-MEM2REG: debug_value %1 : $Int64, var, (name "my_struct", loc "sroa.swift":8:9
+  // CHECK-MEM2REG-SAME: type $*MyStruct, expr op_fragment:#MyStruct.y
+  // CHECK-MEM2REG: debug_value %1 : $Int64, let, (name "my_copy", loc "sroa.swift":7:10
+  // CHECK-MEM2REG-SAME: type $MyStruct, expr op_fragment:#MyStruct.y
   return %0 : $Int64, loc "sroa.swift":11:5, scope 2
 } // end sil function 'foo'

test/SILOptimizer/mem2reg_lifetime_nontrivial.sil

@@ -595,8 +595,8 @@ bb3:
 // CHECK-NOT:   alloc_stack
 // CHECK-NOT:   debug_value {{.*}} expr op_deref
 // CHECK:       {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $Klass):
+// CHECK:         debug_value [[INSTANCE]]
 // CHECK:         [[LIFETIME_OWNED:%[^,]+]] = move_value [lexical] [[INSTANCE]]
-// CHECK:         debug_value [[LIFETIME_OWNED]]
 // CHECK-LABEL: } // end sil function 'mem2reg_debug_value'
 sil [ossa] @mem2reg_debug_value : $@convention(thin) (@owned Klass) -> @owned Klass {
 bb0(%0 : @owned $Klass):

test/SILOptimizer/mem2reg_resilient_lifetime.sil

@@ -11,17 +11,19 @@ public struct ResilientStruct {
 // CHECK-LABEL: sil [ossa] @mem2reg_debug_value :
 // CHECK:       {{bb[^,]+}}([[REGISTER_0:%[^,]+]] : $*ResilientStruct):
 // CHECK-NEXT:    [[REGISTER_1:%[^,]+]] = load [copy] [[REGISTER_0]]
+// CHECK-NEXT:    debug_value [[REGISTER_1]] : $ResilientStruct, var, name "before"
 // CHECK-NEXT:    [[REGISTER_3:%[^,]+]] = move_value [lexical] [[REGISTER_1]]
-// CHECK-NEXT:    debug_value [[REGISTER_3]]
+// CHECK-NEXT:    debug_value [[REGISTER_3]] : $ResilientStruct, let, name "after"
 // CHECK-NEXT:    destroy_value [[REGISTER_3]]
 // CHECK-LABEL: } // end sil function 'mem2reg_debug_value'
 sil [ossa] @mem2reg_debug_value : $@convention(thin) (@in_guaranteed ResilientStruct) -> () {
 bb0(%0 : $*ResilientStruct):
   %1 = alloc_stack [lexical] $ResilientStruct
   %2 = load [copy] %0 : $*ResilientStruct
   store %2 to [init] %1 : $*ResilientStruct
-  debug_value %1 : $*ResilientStruct, expr op_deref
+  debug_value %1 : $*ResilientStruct, var, name "before", expr op_deref
   %3 = load [take] %1 : $*ResilientStruct
+  debug_value %3 : $ResilientStruct, let, name "after"
   destroy_value %3 : $ResilientStruct
   dealloc_stack %1 : $*ResilientStruct
   %4 = tuple ()