Merge pull request #73387 from Snowy1803/allocbox-fix

[DebugInfo] Fix loss of variables at -Onone

Author: Snowy1803

docs/DebuggingTheCompiler.md

@@ -260,7 +260,7 @@ If one builds swift using ninja and wants to dump the SIL of the
 stdlib using some of the SIL dumping options from the previous
 section, one can use the following one-liner:
 
-    ninja -t commands | grep swiftc | grep Swift.o | grep " -c "
+    ninja -t commands | grep swiftc | grep 'Swift\.o'
 
 This should give one a single command line that one can use for
 Swift.o, perfect for applying the previous sections options to.

docs/HowToUpdateDebugInfo.md

@@ -16,6 +16,11 @@ merge drop and copy locations, since all the same considerations apply. Helpers
 like `SILBuilderWithScope` make it easy to copy source locations when expanding
 SIL instructions.
 
+> [!Warning]
+> Don't use `SILBuilderWithScope` when replacing a single instruction of type
+> `AllocStackInst` or `DebugValueInst`. These meta instructions are skipped,
+> so the wrong scope will be inferred.
+
 ## Variables
 
 Each `debug_value` (and variable-carrying instruction) defines an update point
@@ -254,16 +259,34 @@ debug_value %1 : $Int, var, name "pair", type $Pair, expr op_fragment:#Pair.a //
 ```
 
 ## Rules of thumb
-- Optimization passes may never drop a variable entirely. If a variable is
-  entirely optimized away, an `undef` debug value should still be kept. The only
-  exception is an unreachable function or scope, which is entirely removed.
-- A `debug_value` must always describe a correct value for that source variable
-  at that source location. If a value is only correct on some paths through that
-  instruction, it must be replaced with `undef`. Debug info never speculates.
-- When a SIL instruction is deleted, call salvageDebugInfo(). It will try to
-  capture the effect of the deleted instruction in a debug expression, so the
-  location can be preserved. You can also use an `InstructionDeleter` which will
-  automatically call `salvageDebugInfo`.
+
+### Correctness
+A `debug_value` must always describe a correct value for that source variable
+at that source location. If a value is only correct on some paths through that
+instruction, it must be replaced with `undef`. Debug info never speculates.
+
+### Don't drop debug info
+
+Optimization passes may never drop a variable entirely. If a variable is
+entirely optimized away, an `undef` debug value should still be kept. The only
+exception is when the variable is in an unreachable function or scope, where it
+can be removed with the rest of the instructions.
+
+### Instruction Deletion
+
+When a SIL instruction is deleted, call `salvageDebugInfo`. It will try to
+capture the effect of the deleted instruction in a debug expression, so the
+location can be preserved.
+
+Alternatively, you can use an `InstructionDeleter`, which will automatically
+call `salvageDebugInfo`.
+
+If the debug info cannot be salvaged by `salvageDebugInfo`, and the pass has a
+special knowledge of the value, the pass can directly replace the debug value
+with the known value.
+
+If an instruction is being replaced by another, use `replaceAllUsesWith`. It
+will also update debug values to use the new instruction.
 
 > [!Tip]
 > To detect when a pass drops a variable, you can use the

lib/IRGen/AllocStackHoisting.cpp

@@ -157,9 +157,8 @@ void moveAllocStackToBeginningOfBlock(
   // of the debug_value to the original position.
   if (haveMovedElt) {
     if (auto varInfo = AS->getVarInfo()) {
-      SILBuilderWithScope Builder(AS);
       // SILBuilderWithScope skips over meta instructions when picking a scope.
-      Builder.setCurrentDebugScope(AS->getDebugScope());
+      SILBuilder Builder(AS, AS->getDebugScope());
       auto *DVI = Builder.createDebugValue(AS->getLoc(), AS, *varInfo);
       DVI->setUsesMoveableValueDebugInfo();
       DebugValueToBreakBlocksAt.push_back(DVI);
@@ -198,14 +197,14 @@ void Partition::assignStackLocation(
     if (AssignedLoc == AllocStack) continue;
     eraseDeallocStacks(AllocStack);
     AllocStack->replaceAllUsesWith(AssignedLoc);
-    if (hasAtLeastOneMovedElt) {
-      if (auto VarInfo = AllocStack->getVarInfo()) {
-        SILBuilderWithScope Builder(AllocStack);
-        auto *DVI = Builder.createDebugValue(AllocStack->getLoc(), AssignedLoc,
-                                             *VarInfo);
+    if (auto VarInfo = AllocStack->getVarInfo()) {
+      SILBuilder Builder(AllocStack, AllocStack->getDebugScope());
+      auto *DVI = Builder.createDebugValueAddr(AllocStack->getLoc(),
+                                               AssignedLoc, *VarInfo);
+      if (hasAtLeastOneMovedElt) {
         DVI->setUsesMoveableValueDebugInfo();
-        DebugValueToBreakBlocksAt.push_back(DVI);
       }
+      DebugValueToBreakBlocksAt.push_back(DVI);
     }
     AllocStack->eraseFromParent();
   }

lib/IRGen/LoadableByAddress.cpp

@@ -33,6 +33,7 @@
 #include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/BasicBlockOptUtils.h"
+#include "swift/SILOptimizer/Utils/DebugOptUtils.h"
 #include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SILOptimizer/Utils/StackNesting.h"
 #include "llvm/ADT/MapVector.h"
@@ -1778,7 +1779,7 @@ static void rewriteUsesOfSscalar(StructLoweringState &pass,
       createOutlinedCopyCall(copyBuilder, address, dest, pass);
       storeUser->eraseFromParent();
     } else if (auto *dbgInst = dyn_cast<DebugValueInst>(user)) {
-      SILBuilderWithScope dbgBuilder(dbgInst);
+      SILBuilder dbgBuilder(dbgInst, dbgInst->getDebugScope());
       // Rewrite the debug_value to point to the variable in the alloca.
       dbgBuilder.createDebugValueAddr(dbgInst->getLoc(), address,
                                       *dbgInst->getVarInfo());
@@ -2151,9 +2152,8 @@ static void rewriteFunction(StructLoweringState &pass,
       } else {
         assert(currOperand->getType().isAddress() &&
                "Expected an address type");
-        SILBuilderWithScope debugBuilder(instr);
         // SILBuilderWithScope skips over metainstructions.
-        debugBuilder.setCurrentDebugScope(instr->getDebugScope());
+        SILBuilder debugBuilder(instr, instr->getDebugScope());
         debugBuilder.createDebugValueAddr(instr->getLoc(), currOperand,
                                           *instr->getVarInfo());
         instr->getParent()->erase(instr);
@@ -3655,6 +3655,7 @@ class AssignAddressToDef : SILInstructionVisitor<AssignAddressToDef> {
 
     builder.createCopyAddr(load->getLoc(), load->getOperand(), addr, IsTake,
                            IsInitialization);
+    swift::salvageLoadDebugInfo(load);
     assignment.markForDeletion(load);
   }
 

lib/SIL/IR/SILInstruction.cpp

@@ -1478,9 +1478,8 @@ bool SILInstruction::mayTrap() const {
 }
 
 bool SILInstruction::isMetaInstruction() const {
-  // Every instruction that implements getVarInfo() should be in this list.
+  // Every instruction that doesn't generate code should be in this list.
   switch (getKind()) {
-  case SILInstructionKind::AllocBoxInst:
   case SILInstructionKind::AllocStackInst:
   case SILInstructionKind::DebugValueInst:
     return true;

test/DebugInfo/LoadableByAddress.sil

@@ -0,0 +1,49 @@
+// RUN: %target-sil-opt -loadable-address %s | %FileCheck %s
+
+sil_stage canonical
+
+import Builtin
+import Swift
+
+struct X {
+  var x1 : Int
+  var x2 : Int
+  var x3 : Int
+  var x4: Int
+  var x5: Int
+  var x6: Int
+  var x7: Int
+  var x8: Int
+  var x9: Int
+  var x10: Int
+  var x11: Int
+  var x12: Int
+  var x13: Int
+  var x14: Int
+  var x15: Int
+  var x16: Int
+}
+
+
+// CHECK: sil @test1 : $@convention(thin) () -> () {
+// CHECK: bb0:
+// CHECK:   %0 = alloc_stack $Optional<X>
+// CHECK:   %1 = alloc_stack $X
+// CHECK:   %2 = alloc_stack $X
+// CHECK:   %3 = alloc_stack $Optional<X>
+// CHECK:   debug_value %{{[0-9]+}} : $*X, let, name "temp"
+// CHECK: } // end sil function 'test1'
+
+sil @test1 : $@convention(thin) () -> () {
+bb0:
+  %0 = alloc_stack $X
+  %1 = alloc_stack $Optional<X>
+  %2 = load %0 : $*X
+  debug_value %2 : $X, let, name "temp"
+  %3 = enum $Optional<X>, #Optional.some!enumelt, %2 : $X
+  store %3 to %1 : $*Optional<X>
+  dealloc_stack %1 : $*Optional<X>
+  dealloc_stack %0 : $*X
+  %t = tuple ()
+  return %t : $()
+}

test/SILOptimizer/allocstack_hoisting_debuginfo.sil

@@ -98,13 +98,14 @@ bb4:
   return %9999 : $()
 }
 
-// This case we are not moving anything so we are leaving in the default
-// behavior which is not breaking blocks and not inserting debug_info.
+// This case we are not moving anything, so we don't add the
+// [moveable_value_debuginfo] flag, but we still want debug info for the
+// second alloc_stack!
 //
 // CHECK-LABEL: sil @hoist_generic_3 :
 // CHECK: bb0([[ARG:%.*]] : $*T,
 // CHECK-NEXT:   [[STACK:%.*]] = alloc_stack $T{{[ ]*}}, let, name "x"
-// CHECK-NOT:    debug_value
+// CHECK:        debug_value %{{.+}}, let, name "y"
 // CHECK: } // end sil function 'hoist_generic_3'
 sil @hoist_generic_3 : $@convention(thin) <T> (@in T, Builtin.Int1) -> () {
 bb0(%0 : $*T, %1: $Builtin.Int1):