Debug Info: Add missing debug info propagation to SILCloner.

While tightening the requirements of the debug info generator in
IRGenSIL I noticed that SILCloner didn't correctly transfer variable
debug info on alloc_box and alloc_stack instructions. In order to make
these mistakes easier to find I added an assertion to SILBuilder and
fixed all issues uncovered by that assertion, too.

The result is a moderate increase in debug info coverage in optimized code.

On stdlib/public/core/OSX/x86_64/Swift.o "variables with location"
increases from 60134 to 60299.

Author: adrian-prantl

include/swift/SIL/SILBuilder.h

@@ -403,6 +403,8 @@ class SILBuilder {
                                    Optional<SILDebugVariable> Var = None,
                                    bool hasDynamicLifetime = false) {
     Loc.markAsPrologue();
+    assert((!dyn_cast_or_null<VarDecl>(Loc.getAsASTNode<Decl>()) || Var) &&
+           "location is a VarDecl, but SILDebugVariable is empty");
     return insert(AllocStackInst::create(getSILDebugLocation(Loc), elementType,
                                          getFunction(), C.OpenedArchetypes,
                                          Var, hasDynamicLifetime));
@@ -443,6 +445,8 @@ class SILBuilder {
                                Optional<SILDebugVariable> Var = None,
                                bool hasDynamicLifetime = false) {
     Loc.markAsPrologue();
+    assert((!dyn_cast_or_null<VarDecl>(Loc.getAsASTNode<Decl>()) || Var) &&
+           "location is a VarDecl, but SILDebugVariable is empty");
     return insert(AllocBoxInst::create(getSILDebugLocation(Loc), BoxType, *F,
                                        C.OpenedArchetypes, Var,
                                        hasDynamicLifetime));

include/swift/SIL/SILCloner.h

@@ -784,9 +784,16 @@ template<typename ImplClass>
 void
 SILCloner<ImplClass>::visitAllocStackInst(AllocStackInst *Inst) {
   getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
-  recordClonedInstruction(
-      Inst, getBuilder().createAllocStack(getOpLocation(Inst->getLoc()),
-                                          getOpType(Inst->getElementType())));
+  // Drop the debug info from mandatory-inlined instructions. It's the law!
+  SILLocation Loc = getOpLocation(Inst->getLoc());
+  Optional<SILDebugVariable> VarInfo = Inst->getVarInfo();
+  if (Loc.getKind() == SILLocation::MandatoryInlinedKind) {
+    Loc = MandatoryInlinedLocation::getAutoGeneratedLocation();
+    VarInfo = None;
+  }
+  recordClonedInstruction(Inst,
+                          getBuilder().createAllocStack(
+                              Loc, getOpType(Inst->getElementType()), VarInfo));
 }
 
 template<typename ImplClass>
@@ -829,11 +836,19 @@ template<typename ImplClass>
 void
 SILCloner<ImplClass>::visitAllocBoxInst(AllocBoxInst *Inst) {
   getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
+  // Drop the debug info from mandatory-inlined instructions.
+  SILLocation Loc = getOpLocation(Inst->getLoc());
+  Optional<SILDebugVariable> VarInfo = Inst->getVarInfo();
+  if (Loc.getKind() == SILLocation::MandatoryInlinedKind) {
+    Loc = MandatoryInlinedLocation::getAutoGeneratedLocation();
+    VarInfo = None;
+  }
+
   recordClonedInstruction(
       Inst,
       getBuilder().createAllocBox(
-          getOpLocation(Inst->getLoc()),
-          this->getOpType(Inst->getType()).template castTo<SILBoxType>()));
+          Loc, this->getOpType(Inst->getType()).template castTo<SILBoxType>(),
+          VarInfo));
 }
 
 template<typename ImplClass>

include/swift/SIL/SILInstruction.h

@@ -1315,14 +1315,18 @@ class UnaryInstructionWithTypeDependentOperandsBase
 /// arguments that are needed by DebugValueInst, DebugValueAddrInst,
 /// AllocStackInst, and AllocBoxInst.
 struct SILDebugVariable {
+  StringRef Name;
+  unsigned ArgNo : 16;
+  unsigned Constant : 1;
+
   SILDebugVariable() : ArgNo(0), Constant(false) {}
   SILDebugVariable(bool Constant, uint16_t ArgNo)
       : ArgNo(ArgNo), Constant(Constant) {}
   SILDebugVariable(StringRef Name, bool Constant, unsigned ArgNo)
       : Name(Name), ArgNo(ArgNo), Constant(Constant) {}
-  StringRef Name;
-  unsigned ArgNo : 16;
-  unsigned Constant : 1;
+  bool operator==(const SILDebugVariable &V) {
+    return ArgNo == V.ArgNo && Constant == V.Constant && Name == V.Name;
+  }
 };
 
 /// A DebugVariable where storage for the strings has been

include/swift/SIL/SILLocation.h

@@ -662,6 +662,7 @@ class MandatoryInlinedLocation : public SILLocation {
       : SILLocation(L, MandatoryInlinedKind) {}
 
   static MandatoryInlinedLocation getMandatoryInlinedLocation(SILLocation L);
+  static MandatoryInlinedLocation getAutoGeneratedLocation();
 
   static MandatoryInlinedLocation getModuleLocation(unsigned Flags) {
     auto L = MandatoryInlinedLocation();

lib/IRGen/IRGenSIL.cpp

@@ -4055,6 +4055,8 @@ void IRGenSILFunction::visitAllocStackInst(swift::AllocStackInst *i) {
   if (!Decl)
     return;
 
+  assert(i->getVarInfo() && "alloc_stack without debug info");
+
   Type Desugared = Decl->getType()->getDesugaredType();
   if (Desugared->getClassOrBoundGenericClass() ||
       Desugared->getStructOrBoundGenericStruct())
@@ -4214,6 +4216,9 @@ void IRGenSILFunction::visitAllocBoxInst(swift::AllocBoxInst *i) {
 
   if (!Decl)
     return;
+
+  assert(i->getVarInfo() && "alloc_box without debug info");
+  
   // FIXME: This is a workaround to not produce local variables for
   // capture list arguments like "[weak self]". The better solution
   // would be to require all variables to be described with a

lib/IRGen/LoadableByAddress.cpp

@@ -1040,18 +1040,23 @@ class LoadableStorageAllocation {
 };
 } // end anonymous namespace
 
-static AllocStackInst *allocate(StructLoweringState &pass,
-                                SILLocation loc, SILType type) {
+static AllocStackInst *allocate(StructLoweringState &pass, SILType type) {
   assert(type.isObject());
 
   // Insert an alloc_stack at the beginning of the function.
   SILBuilderWithScope allocBuilder(&*pass.F->begin());
-  AllocStackInst *alloc = allocBuilder.createAllocStack(loc, type);
+  // Don't put any variable debug info into the alloc_stack, there will be a
+  // debug_value_addr insterted later. TODO: It may be more elegant to insert
+  // the variable info into the alloc_stack instead of additionally generating a
+  // debug_value_addr.
+  AllocStackInst *alloc = allocBuilder.createAllocStack(
+      RegularLocation::getAutoGeneratedLocation(), type);
 
   // Insert dealloc_stack at the end(s) of the function.
   for (TermInst *termInst : pass.returnInsts) {
     SILBuilderWithScope deallocBuilder(termInst);
-    deallocBuilder.createDeallocStack(loc, alloc);
+    deallocBuilder.createDeallocStack(
+        RegularLocation::getAutoGeneratedLocation(), alloc);
   }
 
   return alloc;
@@ -1091,8 +1096,7 @@ void LoadableStorageAllocation::replaceLoadWithCopyAddr(
     LoadInst *optimizableLoad) {
   SILValue value = optimizableLoad->getOperand();
 
-  auto allocInstr = allocate(pass, value.getLoc(),
-                             value->getType().getObjectType());
+  auto allocInstr = allocate(pass, value->getType().getObjectType());
 
   SILBuilderWithScope outlinedBuilder(optimizableLoad);
   createOutlinedCopyCall(outlinedBuilder, value, allocInstr, pass);
@@ -1214,8 +1218,7 @@ void LoadableStorageAllocation::replaceLoadWithCopyAddrForModifiable(
   }
   SILValue value = unoptimizableLoad->getOperand();
 
-  AllocStackInst *alloc = allocate(pass, value.getLoc(),
-                                   value->getType().getObjectType());
+  AllocStackInst *alloc = allocate(pass, value->getType().getObjectType());
 
   SILBuilderWithScope outlinedBuilder(unoptimizableLoad);
   createOutlinedCopyCall(outlinedBuilder, value, alloc, pass);
@@ -1577,8 +1580,8 @@ void LoadableStorageAllocation::allocateForArg(SILValue value) {
   SILBuilderWithScope allocBuilder(&*pass.F->begin()->begin(),
                                    FirstNonAllocStack);
 
-  AllocStackInst *allocInstr =
-      allocBuilder.createAllocStack(value.getLoc(), value->getType());
+  AllocStackInst *allocInstr = allocBuilder.createAllocStack(
+      RegularLocation::getAutoGeneratedLocation(), value->getType());
 
   LoadInst *loadCopy = nullptr;
   auto *applyOutlinedCopy =
@@ -1604,7 +1607,12 @@ AllocStackInst *
 LoadableStorageAllocation::allocateForApply(SILInstruction *apply,
                                             SILType type) {
   SILBuilderWithScope allocBuilder(&*pass.F->begin());
-  auto *allocInstr = allocBuilder.createAllocStack(apply->getLoc(), type);
+  SILLocation Loc = apply->getLoc();
+  if (dyn_cast_or_null<VarDecl>(Loc.getAsASTNode<Decl>()))
+    // FIXME: Remove this. This is likely indicative of a bug earlier in the
+    // pipeline. An apply instruction should not have a VarDecl as location.
+    Loc = RegularLocation::getAutoGeneratedLocation();
+  auto *allocInstr = allocBuilder.createAllocStack(Loc, type);
 
   pass.largeLoadableArgs.push_back(allocInstr);
   pass.allocToApplyRetMap[allocInstr] = apply;
@@ -1713,7 +1721,7 @@ static void rewriteUsesOfSscalar(StructLoweringState &pass,
 static void allocateAndSetForInstResult(StructLoweringState &pass,
                                         SILValue instResult,
                                         SILInstruction *inst) {
-  auto alloc = allocate(pass, inst->getLoc(), instResult->getType());
+  auto alloc = allocate(pass, instResult->getType());
 
   auto II = inst->getIterator();
   ++II;
@@ -1726,7 +1734,7 @@ static void allocateAndSetForInstResult(StructLoweringState &pass,
 static void allocateAndSetForArgument(StructLoweringState &pass,
                                       SILArgument *value,
                                       SILInstruction *user) {
-  AllocStackInst *alloc = allocate(pass, user->getLoc(), value->getType());
+  AllocStackInst *alloc = allocate(pass, value->getType());
 
   SILLocation loc = user->getLoc();
   loc.markAutoGenerated();
@@ -1871,12 +1879,12 @@ static SILValue createCopyOfEnum(StructLoweringState &pass,
   auto type = value->getType();
   if (type.isObject()) {
     // support for non-address operands / enums
-    auto *alloc = allocate(pass, orig->getLoc(), type);
+    auto *alloc = allocate(pass, type);
     createStoreInit(pass, orig->getIterator(), orig->getLoc(), value, alloc);
     return alloc;
   }
 
-  auto alloc = allocate(pass, value.getLoc(), type.getObjectType());
+  auto alloc = allocate(pass, type.getObjectType());
 
   SILBuilderWithScope copyBuilder(orig);
   createOutlinedCopyCall(copyBuilder, value, alloc, pass);

lib/SIL/SILLocation.cpp

@@ -223,6 +223,11 @@ MandatoryInlinedLocation::getMandatoryInlinedLocation(SILLocation L) {
   llvm_unreachable("Cannot construct Inlined loc from the given location.");
 }
 
+MandatoryInlinedLocation MandatoryInlinedLocation::getAutoGeneratedLocation() {
+  return getMandatoryInlinedLocation(
+      RegularLocation::getAutoGeneratedLocation());
+}
+
 CleanupLocation CleanupLocation::get(SILLocation L) {
   if (Expr *E = L.getAsASTNode<Expr>())
     return CleanupLocation(E, L.getSpecialFlags());

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

@@ -134,7 +134,7 @@ void ExistentialSpecializerCloner::cloneArguments(
   // SILBuilderContext.
   SILBuilder NewFBuilder(ClonedEntryBB, DebugScope,
                          getBuilder().getBuilderContext());
-  auto InsertLoc = OrigF->begin()->begin()->getLoc();
+  auto InsertLoc = RegularLocation::getAutoGeneratedLocation();
 
   auto NewFTy = NewF.getLoweredFunctionType();
   SmallVector<SILParameterInfo, 4> params;

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -299,9 +299,15 @@ promoteDebugValueAddr(DebugValueAddrInst *DVAI, SILValue Value, SILBuilder &B) {
   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.
+  for (Operand *Use : Value->getUses())
+    if (auto *DVI = dyn_cast<DebugValueInst>(Use->getUser()))
+      if (*DVI->getVarInfo() == *DVAI->getVarInfo())
+        return;
   B.setInsertionPoint(DVAI);
   B.setCurrentDebugScope(DVAI->getDebugScope());
   B.createDebugValue(DVAI->getLoc(), Value, *DVAI->getVarInfo());
+
   DVAI->eraseFromParent();
 }
 

lib/SILOptimizer/Transforms/SILSROA.cpp

@@ -215,19 +215,21 @@ SROAMemoryUseAnalyzer::
 createAllocas(llvm::SmallVector<AllocStackInst *, 4> &NewAllocations) {
   SILBuilderWithScope B(AI);
   SILType Type = AI->getType().getObjectType();
-
+  // Intentionally dropping the debug info.
+  // FIXME: VarInfo needs to be extended with fragment info to support this.
+  SILLocation Loc = RegularLocation::getAutoGeneratedLocation();
   if (TT) {
     for (unsigned EltNo : indices(TT->getElementTypes())) {
       SILType EltTy = Type.getTupleElementType(EltNo);
-      NewAllocations.push_back(B.createAllocStack(AI->getLoc(), EltTy));
+      NewAllocations.push_back(B.createAllocStack(Loc, EltTy, {}));
     }
   } else {
     assert(SD && "SD should not be null since either it or TT must be set at "
            "this point.");
     SILModule &M = AI->getModule();
     for (auto *D : SD->getStoredProperties())
-      NewAllocations.push_back(B.createAllocStack(AI->getLoc(),
-                                                  Type.getFieldType(D, M)));
+      NewAllocations.push_back(
+          B.createAllocStack(Loc, Type.getFieldType(D, M), {}));
   }
 }
 

lib/SILOptimizer/Utils/GenericCloner.cpp

@@ -81,7 +81,8 @@ void GenericCloner::populateCloned() {
       // We need an alloc_stack as a replacement for the indirect parameter.
       assert(mappedType.isAddress());
       mappedType = mappedType.getObjectType();
-      ASI = getBuilder().createAllocStack(Loc, mappedType);
+      auto AllocStackLoc = RegularLocation::getAutoGeneratedLocation();
+      ASI = getBuilder().createAllocStack(AllocStackLoc, mappedType);
       AllocStacks.push_back(ASI);
     };
     auto handleConversion = [&]() {

test/DebugInfo/inlined-generics-basic.swift

@@ -67,15 +67,15 @@ public class C<R> {
     // IR: call {{.*}}3use
 #sourceLocation(file: "f.swift", line: 4)
     g((r, s))
-    // IR: dbg.value
-    // IR: dbg.value({{.*}}, metadata ![[GI_T:[0-9]+]]
+    // Note to maintainers: the relative order of the constant dbg.values here
+    // seem to flip back and forth.
     // IR: dbg.value({{.*}}, metadata ![[GI_U:[0-9]+]]
+    // IR: dbg.value({{.*}}, metadata ![[GI_T:[0-9]+]]
     // IR: call {{.*}}3use
 #sourceLocation(file: "f.swift", line: 5)
     g(Int(0))
-    // IR: dbg.value
-    // IR: dbg.value({{.*}}, metadata ![[GB_T:[0-9]+]]
     // IR: dbg.value({{.*}}, metadata ![[GB_U:[0-9]+]]
+    // IR: dbg.value({{.*}}, metadata ![[GB_T:[0-9]+]]
     // IR: call {{.*}}3use
 #sourceLocation(file: "f.swift", line: 6)
     g(false)

test/SILOptimizer/sil_locations.sil

@@ -49,7 +49,7 @@ bb0(%0 : $Int64):
   // CHECK-LABEL: sil @inline_test_add : $@convention(thin) (Int64) -> Int64 {
   // CHECK: [[VAL9:%.*]] = apply                               {{.*}} line:38:9:sil
   // CHECK: [[VAL10:%.*]] = apply                         {{.*}} line:39:9:sil
-  // CHECK: [[VAL11:%.*]] = alloc_box $<τ_0_0> { var τ_0_0 } <Int64>,             {{.*}} line:40:9:minlined
+  // CHECK: [[VAL11:%.*]] = alloc_box $<τ_0_0> { var τ_0_0 } <Int64>,             {{.*}}compiler-generated
   // CHECK: [[PB11:%.*]] = project_box [[VAL11]]          {{.*}} line:40:9:minlined
   // CHECK: store [[VAL10]] to [[PB11]]                   {{.*}} line:40:9:minlined
   // CHECK: [[VAL13:%.*]] = function_ref @plus            {{.*}} line:40:9:minlined

test/SILOptimizer/specialize_unconditional_checked_cast.swift

@@ -100,7 +100,6 @@ ArchetypeToConcreteConvertUInt8(t: f)
 // CHECK-LABEL: sil shared [noinline] @$s37specialize_unconditional_checked_cast31ArchetypeToConcreteConvertUInt8{{[_0-9a-zA-Z]*}}3Not{{.*}}Tg5 : $@convention(thin) (NotUInt8) -> NotUInt8 {
 // CHECK: bb0
 // CHECK-NEXT: debug_value %0
-// CHECK-NEXT: debug_value %0
 // CHECK-NEXT: return %0
 
 // x -> y where y is a class but x is not.
@@ -121,7 +120,6 @@ ArchetypeToConcreteConvertUInt8(t: f)
 // CHECK-LABEL: sil shared [noinline] @$s37specialize_unconditional_checked_cast27ArchetypeToConcreteConvertC{{[_0-9a-zA-Z]*}}Tg5 : $@convention(thin) (@guaranteed C) -> @owned C {
 // CHECK: bb0([[ARG:%.*]] : $C)
 // CHECK-NEXT: debug_value [[ARG]]
-// CHECK-NEXT: debug_value [[ARG]]
 // CHECK-NEXT: strong_retain [[ARG]]
 // CHECK-NEXT: return [[ARG]]