Merge pull request swiftlang#71925 from Snowy1803/recursive-sroa

[DebugInfo] Add debug info support for recursive SIL SROA

Author: adrian-prantl

include/swift/SIL/SILDebugVariable.h

@@ -76,6 +76,10 @@ struct SILDebugVariable {
   static std::optional<SILDebugVariable>
   createFromAllocation(const AllocationInst *AI);
 
+  /// Return the underlying variable declaration that this denotes,
+  /// or nullptr if we don't have one.
+  VarDecl *getDecl() const;
+
   // We're not comparing DIExpr here because strictly speaking,
   // DIExpr is not part of the debug variable. We simply piggyback
   // it in this class so that's it's easier to carry DIExpr around.

lib/IRGen/IRGenDebugInfo.cpp

@@ -2961,21 +2961,19 @@ bool IRGenDebugInfoImpl::buildDebugInfoExpression(
     llvm::DIExpression::FragmentInfo &Fragment) {
   assert(VarInfo.DIExpr && "SIL debug info expression not found");
 
-#ifndef NDEBUG
-  bool HasFragment = VarInfo.DIExpr.hasFragment();
-#endif
-
   const auto &DIExpr = VarInfo.DIExpr;
   for (const SILDIExprOperand &ExprOperand : DIExpr.operands()) {
+    llvm::DIExpression::FragmentInfo SubFragment = {0, 0};
     switch (ExprOperand.getOperator()) {
     case SILDIExprOperator::Fragment:
-      assert(HasFragment && "Fragment must be the last part of a DIExpr");
-#ifndef NDEBUG
-      // Trigger the assert above if we have more than one fragment expression.
-      HasFragment = false;
-#endif
-      if (!handleFragmentDIExpr(ExprOperand, Fragment))
+      if (!handleFragmentDIExpr(ExprOperand, SubFragment))
         return false;
+      assert(!Fragment.SizeInBits
+             || (SubFragment.OffsetInBits + SubFragment.SizeInBits
+              <= Fragment.SizeInBits)
+             && "Invalid nested fragments");
+      Fragment.OffsetInBits += SubFragment.OffsetInBits;
+      Fragment.SizeInBits = SubFragment.SizeInBits;
       break;
     case SILDIExprOperator::Dereference:
       Operands.push_back(llvm::dwarf::DW_OP_deref);

lib/IRGen/IRGenSIL.cpp

@@ -5463,9 +5463,16 @@ void IRGenSILFunction::visitDebugValueInst(DebugValueInst *i) {
       assert(isa<SILBoxType>(RealTy));
     }
 
+  VarDecl *VD = i->getDecl();
+  if (!VD) {
+    // The source location of a DebugValueInst inserted by the SIL optimizer is
+    // not necessarily the VarDecl, as it can be the source location of the
+    // update point this DebugValueInst represents.
+    VD = VarInfo->getDecl();
+  }
   // Figure out the debug variable type
-  if (VarDecl *Decl = i->getDecl()) {
-    DbgTy = DebugTypeInfo::getLocalVariable(Decl, RealTy, getTypeInfo(SILTy),
+  if (VD) {
+    DbgTy = DebugTypeInfo::getLocalVariable(VD, RealTy, getTypeInfo(SILTy),
                                             IGM, IsFragmentType);
   } else if (!SILTy.hasArchetype() && !VarInfo->Name.empty()) {
     // Handle the cases that read from a SIL file

lib/SIL/IR/SILInstructions.cpp

@@ -208,10 +208,6 @@ SILDebugVariable::createFromAllocation(const AllocationInst *AI) {
   if (!VarInfo)
     return {};
 
-  // TODO: Support variables with expressions.
-  if (VarInfo->DIExpr)
-    return {};
-
   // Coalesce the debug loc attached on AI into VarInfo
   SILType Type = AI->getType();
   SILLocation InstLoc = AI->getLoc();
@@ -495,6 +491,12 @@ VarDecl *DebugValueInst::getDecl() const {
   return getLoc().getAsASTNode<VarDecl>();
 }
 
+VarDecl *SILDebugVariable::getDecl() const {
+  if (!Loc)
+    return nullptr;
+  return Loc->getAsASTNode<VarDecl>();
+}
+
 AllocExistentialBoxInst *AllocExistentialBoxInst::create(
     SILDebugLocation Loc, SILType ExistentialType, CanType ConcreteType,
     ArrayRef<ProtocolConformanceRef> Conformances,

lib/SIL/Verifier/SILVerifier.cpp

@@ -1469,6 +1469,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
 
     // Check debug info expression
     if (const auto &DIExpr = varInfo->DIExpr) {
+      bool HasFragment = false;
       for (auto It = DIExpr.element_begin(), ItEnd = DIExpr.element_end();
            It != ItEnd;) {
         require(It->getKind() == SILDIExprElement::OperatorKind,
@@ -1483,8 +1484,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                   "di-expression operand kind mismatch");
 
         if (Op == SILDIExprOperator::Fragment)
-          require(It == ItEnd, "op_fragment directive needs to be at the end "
-                               "of a di-expression");
+          HasFragment = true;
+        else
+          require(!HasFragment, "no directive allowed after op_fragment"
+                  " in a di-expression");
       }
     }
   }

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -1854,12 +1854,6 @@ void swift::salvageDebugInfo(SILInstruction *I) {
       auto VarInfo = DbgInst->getVarInfo();
       if (!VarInfo)
         continue;
-      if (VarInfo->DIExpr.hasFragment())
-        // Since we can't merge two different op_fragment
-        // now, we're simply bailing out if there is an
-        // existing op_fragment in DIExpression.
-        // TODO: Try to merge two op_fragment expressions here.
-        continue;
       for (VarDecl *FD : FieldDecls) {
         SILDebugVariable NewVarInfo = *VarInfo;
         auto FieldVal = STI->getFieldValue(FD);
@@ -1933,10 +1927,7 @@ void swift::createDebugFragments(SILValue oldValue, Projection proj,
     if (!debugVal)
       continue;
 
-    // Can't create a fragment of a fragment.
     auto varInfo = debugVal->getVarInfo();
-    if (!varInfo || varInfo->DIExpr.hasFragment())
-      continue;
 
     SILType baseType = oldValue->getType();
 

test/DebugInfo/nested_salvage_struct.sil

@@ -0,0 +1,87 @@
+// RUN: %target-sil-opt %s -performance-constant-propagation | %FileCheck %s
+
+sil_stage canonical
+
+import Builtin
+import Swift
+
+struct Wrapper {
+  let bytes: UInt64
+  private func pop(numBits: UInt64) -> Wrapper
+  public static func maker(bits: UInt64) -> Wrapper
+}
+
+sil_scope 3 { loc "e.swift":6:16 parent @pop : $@convention(method) (UInt64, Wrapper) -> Wrapper }
+sil_scope 4 { loc "e.swift":8:12 parent 3 }
+
+// Wrapper.pop(numBits:), scope 3
+sil private @pop : $@convention(method) (UInt64, Wrapper) -> Wrapper {
+[global: ]
+bb0(%0 : $UInt64, %1 : $Wrapper):
+  debug_value %0 : $UInt64, let, name "numBits", argno 1, scope 3
+  debug_value %1 : $Wrapper, let, name "self", argno 2, implicit, scope 3
+  %4 = struct_extract %1 : $Wrapper, #Wrapper.bytes, scope 3
+  %5 = struct_extract %4 : $UInt64, #UInt64._value, scope 3
+  %6 = struct_extract %0 : $UInt64, #UInt64._value, scope 3
+  %7 = integer_literal $Builtin.Int1, -1, scope 3
+  %8 = builtin "usub_with_overflow_Int64"(%5 : $Builtin.Int64, %6 : $Builtin.Int64, %7 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1), scope 3
+  %9 = tuple_extract %8 : $(Builtin.Int64, Builtin.Int1), 0, scope 3
+  %10 = tuple_extract %8 : $(Builtin.Int64, Builtin.Int1), 1, scope 3
+  cond_fail %10 : $Builtin.Int1, "arithmetic overflow", scope 3
+  %12 = struct $UInt64 (%9 : $Builtin.Int64), scope 3
+  %13 = struct $Wrapper (%12 : $UInt64), loc * "e.swift":3:8
+  return %13 : $Wrapper, scope 3
+} // end sil function 'pop'
+
+sil_scope 7 { loc "e.swift":12:22 parent @maker : $@convention(method) (UInt64, @thin Wrapper.Type) -> Wrapper }
+sil_scope 8 { loc "e.swift":17:12 parent 7 }
+sil_scope 10 { loc "e.swift":17:39 parent 7 }
+sil_scope 11 { loc "e.swift":6:16 parent @pop : $@convention(method) (UInt64, Wrapper) -> Wrapper inlined_at 10 }
+sil_scope 12 { loc "e.swift":8:12 parent 11 inlined_at 10 }
+
+// static Wrapper.maker(bits:), scope 7
+// CHECK-LABEL: sil {{.*}} @maker
+sil hidden @maker : $@convention(method) (UInt64, @thin Wrapper.Type) -> Wrapper {
+[global: ]
+bb0(%0 : $UInt64, %1 : $@thin Wrapper.Type):
+  // CHECK: debug_value %0 : $UInt64, let, name "bits", argno 1
+  debug_value %0 : $UInt64, let, name "bits", argno 1, scope 7
+  // CHECK: debug_value %1 : $@thin Wrapper.Type, let, name "self", argno 2, implicit
+  debug_value %1 : $@thin Wrapper.Type, let, name "self", argno 2, implicit, scope 7
+  %4 = integer_literal $Builtin.Int64, 3735928559, scope 7
+  %5 = struct $UInt64 (%4 : $Builtin.Int64), scope 7
+  %6 = struct $Wrapper (%5 : $UInt64), loc * "e.swift":3:8
+  // CHECK-DAG: debug_value %0 : $UInt64, let, name "numBits", argno 1
+  debug_value %0 : $UInt64, let, name "numBits", argno 1, scope 11
+  // CHECK-DAG: debug_value %{{.*}} : $Builtin.Int64, let, name "self", {{.*}}, implicit, type $Wrapper, expr op_fragment:#Wrapper.bytes:op_fragment:#UInt64._value
+  debug_value %6 : $Wrapper, let, name "self", argno 2, implicit, scope 11
+  %9 = struct_extract %6 : $Wrapper, #Wrapper.bytes, scope 11
+  %10 = struct_extract %9 : $UInt64, #UInt64._value, scope 11
+  %11 = struct_extract %0 : $UInt64, #UInt64._value, scope 11
+  %12 = integer_literal $Builtin.Int1, -1, scope 11
+  %13 = builtin "usub_with_overflow_Int64"(%10 : $Builtin.Int64, %11 : $Builtin.Int64, %12 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1), scope 11
+  %14 = tuple_extract %13 : $(Builtin.Int64, Builtin.Int1), 0, scope 11
+  %15 = tuple_extract %13 : $(Builtin.Int64, Builtin.Int1), 1, scope 11
+  cond_fail %15 : $Builtin.Int1, "arithmetic overflow", scope 11
+  %17 = struct $UInt64 (%14 : $Builtin.Int64), scope 11
+  %18 = struct $Wrapper (%17 : $UInt64), loc * "e.swift":3:8
+  return %18 : $Wrapper, scope 7
+} // end sil function 'maker'
+
+
+// Generated and adapted from this Swift source code:
+// ```swift
+// struct Wrapper {
+//   let bytes: UInt64
+//
+//   private func pop(numBits: UInt64) -> Wrapper {
+//     // `self` in here is a nested fragment. It should be salvaged, when inlined in `maker(bits:)`
+//     return Self(bytes: bytes - numBits)
+//   }
+//
+//   public static func maker(bits: UInt64) -> Wrapper {
+//     // This function is static, but `pop`'s self should be inlined in here
+//     return Wrapper(bytes: 0xDEADBEEF).pop(numBits: bits)
+//   }
+// }
+// ```

test/DebugInfo/sil_based_dbg.swift

@@ -29,13 +29,12 @@ struct TheStruct {
 }
 // CHECK_DBG_SCOPE-LABEL: sil {{.*}}test_debug_scope
 public func test_debug_scope(val : Int) -> Int {
-    // TODO: Repeated SROA of the same variable is currently disabled.
-    // CHECK_DBG_SCOPE: alloc_stack $Builtin.Int{{[0-9]+}}, 
-    // CHECK_DBG_SCOPE-NOT:                          var,
-    // DISABLED_DBG_SCOPE-SAME:                      var, (name "the_struct",
-    // DISABLED_DBG_SCOPE-SAME:                      loc
+    // CHECK_DBG_SCOPE: alloc_stack $Builtin.Int{{[0-9]+}},
+    // CHECK_DBG_SCOPE-SAME:  var, (name "the_struct",
+    // CHECK_DBG_SCOPE-SAME:  loc
+    // CHECK_DBG_SCOPE-SAME:  expr op_fragment:#TheStruct.the_member:op_fragment:#Int._value
     // The auxiliary debug scope should be removed
-    // DISABLED_DBG_SCOPE-NOT:                       scope {{[0-9]+}})
+    // CHECK_DBG_SCOPE-NOT:   scope {{[0-9]+}})
     var the_struct = TheStruct(the_member: 0)
     the_struct.the_member = val + 13
     return the_struct.the_member

test/DebugInfo/srosroa_mem2reg.sil

@@ -31,11 +31,10 @@ bb0(%0 : $Int64, %1 : $Int64):
   debug_value %0 : $Int64, let, name "in_x", argno 1, loc "sroa.swift":7:10, scope 2
   debug_value %1 : $Int64, let, name "in_y", argno 2, loc "sroa.swift":7:21, scope 2
   %4 = alloc_stack $MyStruct, var, name "my_struct", type $*MyContainer, expr op_fragment:#MyContainer.z, loc "sroa.swift":8:9, scope 2
-  // Recursive SROA of already SROA-ed aggregates is not yet supported.
   // CHECK-SROA:      alloc_stack $Int64
-  // CHECK-SROA-NOT:  var,
+  // CHECK-SROA-SAME: var,
   // CHECK-SROA:      alloc_stack $Int64
-  // CHECK-SROA-NOT:  var,
+  // CHECK-SROA-SAME: var,
   // CHECK-SROA:      metatype
   %5 = metatype $@thin MyStruct.Type, loc "sroa.swift":8:21, scope 2
   %6 = integer_literal $Builtin.Int64, 0, loc "sroa.swift":8:33, scope 2

test/DebugInfo/verifier_debug_info_expression.sil

@@ -1,4 +1,4 @@
-// RUN: not --crash %target-swift-frontend %s -sil-verify-all -g -emit-sil
+// RUN: %target-swift-frontend %s -sil-verify-all -g -emit-sil | %FileCheck %s
 import Builtin
 import Swift
 
@@ -11,11 +11,12 @@ sil_scope 1 { loc "file.swift":7:6 parent @test_fragment : $@convention(thin) ()
 
 sil hidden @test_fragment : $@convention(thin) () -> () {
 bb0:
-  %2 = alloc_stack $MyStruct, var, name "my_struct", loc "file.swift":8:9, scope 1
-  %3 = struct_element_addr %2 : $*MyStruct, #MyStruct.x, loc "file.swift":9:17, scope 1
-  // every op_fragment should be the last di-expression operand
-  debug_value %3 : $*Builtin.Int64, var, name "my_struct", expr op_deref:op_fragment:#MyStruct.y:op_fragment:#MyStruct.x
-  dealloc_stack %2 : $*MyStruct
+  %0 = alloc_stack $MyStruct, var, name "my_struct", loc "file.swift":8:9, scope 1
+  %1 = struct_element_addr %0 : $*MyStruct, #MyStruct.x, loc "file.swift":9:17, scope 1
+  // nested op_fragments at the end are allowed
+  // CHECK: debug_value %1 : $*Builtin.Int64, var, name "my_struct", expr op_deref:op_fragment:#MyStruct.y:op_fragment:#MyStruct.x
+  debug_value %1 : $*Builtin.Int64, var, name "my_struct", expr op_deref:op_fragment:#MyStruct.y:op_fragment:#MyStruct.x
+  dealloc_stack %0 : $*MyStruct
   %r = tuple()
   return %r : $()
 }