Merge pull request #65929 from atrick/fix-splitload-debug

Handle loads in salvageDebugInfo and enable it in splitAggregateLoad

Author: atrick

include/swift/SIL/InstructionUtils.h

@@ -152,6 +152,58 @@ RuntimeEffect getRuntimeEffect(SILInstruction *inst, SILType &impactType);
 void findClosuresForFunctionValue(SILValue V,
                                   TinyPtrVector<PartialApplyInst *> &results);
 
+/// An abstraction over LoadInst/LoadBorrowInst so one can handle both types of
+/// load using common code.
+struct LoadOperation {
+  llvm::PointerUnion<LoadInst *, LoadBorrowInst *> value;
+
+  LoadOperation() : value() {}
+  LoadOperation(SILInstruction *input) : value(nullptr) {
+    if (auto *li = dyn_cast<LoadInst>(input)) {
+      value = li;
+      return;
+    }
+
+    if (auto *lbi = dyn_cast<LoadBorrowInst>(input)) {
+      value = lbi;
+      return;
+    }
+  }
+
+  explicit operator bool() const { return !value.isNull(); }
+
+  SingleValueInstruction *getLoadInst() const {
+    if (value.is<LoadInst *>())
+      return value.get<LoadInst *>();
+    return value.get<LoadBorrowInst *>();
+  }
+
+  SingleValueInstruction *operator*() const { return getLoadInst(); }
+
+  const SingleValueInstruction *operator->() const { return getLoadInst(); }
+
+  SingleValueInstruction *operator->() { return getLoadInst(); }
+
+  SILValue getOperand() const {
+    if (value.is<LoadInst *>())
+      return value.get<LoadInst *>()->getOperand();
+    return value.get<LoadBorrowInst *>()->getOperand();
+  }
+
+  /// Return the ownership qualifier of the underlying load if we have a load or
+  /// None if we have a load_borrow.
+  ///
+  /// TODO: Rather than use an optional here, we should include an invalid
+  /// representation in LoadOwnershipQualifier.
+  Optional<LoadOwnershipQualifier> getOwnershipQualifier() const {
+    if (auto *lbi = value.dyn_cast<LoadBorrowInst *>()) {
+      return None;
+    }
+
+    return value.get<LoadInst *>()->getOwnershipQualifier();
+  }
+};
+
 /// Given a polymorphic builtin \p bi that may be generic and thus have in/out
 /// params, stash all of the information needed for either specializing while
 /// inlining or propagating the type in constant propagation.

include/swift/SILOptimizer/Utils/DebugOptUtils.h

@@ -48,6 +48,13 @@ inline void deleteAllDebugUses(SILInstruction *inst,
 /// new `debug_value` instruction before \p I is deleted.
 void salvageDebugInfo(SILInstruction *I);
 
+/// Transfer debug information associated with the result of \p load to the
+/// load's address operand.
+///
+/// TODO: combine this with salvageDebugInfo when it is supported by
+/// optimizations.
+void salvageLoadDebugInfo(LoadOperation load);
+
 /// Create debug_value fragment for a new partial value.
 ///
 /// Precondition: \p oldValue is a struct or class aggregate. \p proj projects a

include/swift/SILOptimizer/Utils/OwnershipOptUtils.h

@@ -349,58 +349,6 @@ class OwnershipReplaceSingleUseHelper {
   }
 };
 
-/// An abstraction over LoadInst/LoadBorrowInst so one can handle both types of
-/// load using common code.
-struct LoadOperation {
-  llvm::PointerUnion<LoadInst *, LoadBorrowInst *> value;
-
-  LoadOperation() : value() {}
-  LoadOperation(SILInstruction *input) : value(nullptr) {
-    if (auto *li = dyn_cast<LoadInst>(input)) {
-      value = li;
-      return;
-    }
-
-    if (auto *lbi = dyn_cast<LoadBorrowInst>(input)) {
-      value = lbi;
-      return;
-    }
-  }
-
-  explicit operator bool() const { return !value.isNull(); }
-
-  SingleValueInstruction *getLoadInst() const {
-    if (value.is<LoadInst *>())
-      return value.get<LoadInst *>();
-    return value.get<LoadBorrowInst *>();
-  }
-
-  SingleValueInstruction *operator*() const { return getLoadInst(); }
-
-  const SingleValueInstruction *operator->() const { return getLoadInst(); }
-
-  SingleValueInstruction *operator->() { return getLoadInst(); }
-
-  SILValue getOperand() const {
-    if (value.is<LoadInst *>())
-      return value.get<LoadInst *>()->getOperand();
-    return value.get<LoadBorrowInst *>()->getOperand();
-  }
-
-  /// Return the ownership qualifier of the underlying load if we have a load or
-  /// None if we have a load_borrow.
-  ///
-  /// TODO: Rather than use an optional here, we should include an invalid
-  /// representation in LoadOwnershipQualifier.
-  Optional<LoadOwnershipQualifier> getOwnershipQualifier() const {
-    if (auto *lbi = value.dyn_cast<LoadBorrowInst *>()) {
-      return None;
-    }
-
-    return value.get<LoadInst *>()->getOwnershipQualifier();
-  }
-};
-
 /// Extend the store_borrow \p sbi's scope such that it encloses \p newUsers.
 bool extendStoreBorrow(StoreBorrowInst *sbi,
                        SmallVectorImpl<Operand *> &newUses,

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

@@ -21,13 +21,13 @@
 #include "swift/SILOptimizer/Utils/CanonicalizeInstruction.h"
 #include "swift/SIL/DebugUtils.h"
 #include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/OwnershipUtils.h"
 #include "swift/SIL/Projection.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SIL/SILFunction.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/Analysis/SimplifyInstruction.h"
 #include "swift/SILOptimizer/Utils/DebugOptUtils.h"
-#include "swift/SILOptimizer/Utils/OwnershipOptUtils.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/Debug.h"
 
@@ -36,10 +36,6 @@ using namespace swift;
 // Tracing within the implementation can also be activated by the pass.
 #define DEBUG_TYPE pass.debugType
 
-llvm::cl::opt<bool> EnableLoadSplittingDebugInfo(
-    "sil-load-splitting-debug-info", llvm::cl::init(false),
-    llvm::cl::desc("Create debug fragments at -O for partial loads"));
-
 // Vtable anchor.
 CanonicalizeInstruction::~CanonicalizeInstruction() {}
 
@@ -301,19 +297,6 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
     }
     pass.notifyNewInstruction(**lastNewLoad);
 
-    // FIXME: This drops debug info at -Onone load-splitting is required at
-    // -Onone for exclusivity diagnostics. Fix this by
-    // 
-    // 1. At -Onone, preserve the original load when pass.preserveDebugInfo is
-    // true, but moving it out of its current access scope and into an "unknown"
-    // access scope, which won't be enforced as an exclusivity violation.
-    //
-    // 2. At -O, create "debug fragments" recover as much debug info as possible
-    // by creating debug_value fragments for each new partial load. Currently
-    // disabled because of LLVM back-end crashes.
-    if (!pass.preserveDebugInfo && EnableLoadSplittingDebugInfo) {
-      createDebugFragments(*loadInst, proj, lastNewLoad->getLoadInst());
-    }
     if (loadOwnership) {
       if (*loadOwnership == LoadOwnershipQualifier::Copy) {
         // Destroy the loaded value wherever the aggregate load was destroyed.
@@ -336,6 +319,10 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
     nextII = killInstruction(extract, nextII, pass);
   }
 
+  // Preserve the original load's debug information.
+  if (pass.preserveDebugInfo) {
+    swift::salvageLoadDebugInfo(loadInst);
+  }
   // Remove the now unused borrows.
   for (auto *borrow : borrows)
     nextII = killInstAndIncidentalUses(borrow, nextII, pass);
@@ -344,9 +331,8 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
   for (auto *destroy : lifetimeEndingInsts)
     nextII = killInstruction(destroy, nextII, pass);
 
-  // FIXME: remove this temporary hack to advance the iterator beyond
-  // debug_value. A soon-to-be merged commit migrates CanonicalizeInstruction to
-  // use InstructionDeleter.
+  // TODO: remove this hack to advance the iterator beyond debug_value and check
+  // SILInstruction::isDeleted() in the caller instead.
   while (nextII != loadInst->getParent()->end()
          && nextII->isDebugInstruction()) {
     ++nextII;

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -1837,6 +1837,10 @@ void swift::endLifetimeAtLeakingBlocks(SILValue value,
 }
 
 // TODO: this currently fails to notify the pass with notifyNewInstruction.
+//
+// TODO: whenever a debug_value is inserted at a new location, check that no
+// other debug_value instructions exist between the old and new location for
+// the same variable.
 void swift::salvageDebugInfo(SILInstruction *I) {
   if (!I)
     return;
@@ -1856,7 +1860,6 @@ void swift::salvageDebugInfo(SILInstruction *I) {
         }
       }
   }
-
   // If a `struct` SIL instruction is "unwrapped" and removed,
   // for instance, in favor of using its enclosed value directly,
   // we need to make sure any of its related `debug_value` instruction
@@ -1921,6 +1924,23 @@ void swift::salvageDebugInfo(SILInstruction *I) {
   }
 }
 
+void swift::salvageLoadDebugInfo(LoadOperation load) {
+  for (Operand *debugUse : getDebugUses(load.getLoadInst())) {
+    // Create a new debug_value rather than reusing the old one so the
+    // SILBuilder adds 'expr(deref)' to account for the indirection.
+    auto *debugInst = cast<DebugValueInst>(debugUse->getUser());
+    auto varInfo = debugInst->getVarInfo();
+    if (!varInfo)
+      continue;
+
+    // The new debug_value must be "hoisted" to the load to ensure that the
+    // address is still valid.
+    SILBuilder(load.getLoadInst(), debugInst->getDebugScope())
+      .createDebugValueAddr(debugInst->getLoc(), load.getOperand(),
+                            varInfo.value());
+  }
+}
+
 // TODO: this currently fails to notify the pass with notifyNewInstruction.
 void swift::createDebugFragments(SILValue oldValue, Projection proj,
                                  SILValue newValue) {

test/SILOptimizer/silgen_cleanup_debug.sil

@@ -0,0 +1,69 @@
+// RUN: %target-sil-opt -opt-mode=none -silgen-cleanup -sil-print-debuginfo -sil-verify-all %s | %FileCheck %s --check-prefix=CHECK --check-prefix=CHECKDEB
+
+// TODO: add -opt-mode=speed after calling salvageLoadDebugInfo() from salvageDebugInfo().
+
+import Builtin
+
+sil_stage raw
+
+struct Int {
+  var _value : Builtin.Int32
+}
+
+// rdar://104700920 (At -Onone preserve debug info after splitting loads)
+//
+// loadFromArg checks that a new debug_value is inserted with "expr op_deref".
+//
+// loadFromStack checks that a new debug_value is insert without op_deref.
+
+sil_scope 10 { loc "./loadFromArg.swift":1:1 parent @loadFromArg : $@convention(thin) (@inout Int) -> Builtin.Int32 }
+sil_scope 11 { loc "./loadFromArg.swift":1:1 parent 10 }
+// CHECK: sil_scope [[ARGSCOPE1:[0-9]+]] { loc "./loadFromArg.swift":1:1 parent @loadFromArg : $@convention(thin) (@inout Int) -> Builtin.Int32 }
+// CHECK: sil_scope [[ARGSCOPE2:[0-9]+]] { loc "./loadFromArg.swift":1:1 parent [[ARGSCOPE1]] }
+
+// CHECK-LABEL: sil [ossa] @loadFromArg : $@convention(thin) (@inout Int) -> Builtin.Int32 {
+// CHECK: bb0(%0 : $*Int):
+// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] %0 : $*Int
+// CHECK: struct_element_addr [[ACCESS]] : $*Int, #Int._value
+// CHECK: load [trivial] %{{.*}} : $*Builtin.Int32, loc "./loadFromArg.swift":2:1, scope [[ARGSCOPE1]]
+// CHECK: debug_value [[ACCESS]] : $*Int, let, name "sVar", expr op_deref, loc "./loadFromArg.swift":3:1, scope [[ARGSCOPE2]]
+// CHECK-LABEL: } // end sil function 'loadFromArg'
+sil [ossa] @loadFromArg : $@convention(thin) (@inout Int) -> Builtin.Int32 {
+bb0(%0 : $*Int):
+  %2 = begin_access [read] [unknown] %0 : $*Int
+  %3 = load [trivial] %2 : $*Int, loc "./loadFromArg.swift":2:1, scope 10
+  end_access %2 : $*Int
+  debug_value %3 : $Int, let, name "sVar", loc "./loadFromArg.swift":3:1, scope 11
+  %6 = struct_extract %3 : $Int, #Int._value
+  return %6 : $Builtin.Int32
+}
+
+sil @storeToStack : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> @out τ_0_0
+
+sil_scope 20 { loc "./loadFromStack.swift":1:1 parent @loadFromStack : $@convention(thin) (Int) -> Builtin.Int32 }
+sil_scope 21 { loc "./loadFromStack.swift":1:1 parent 20 }
+// CHECK: sil_scope [[STACKSCOPE1:[0-9]+]] { loc "./loadFromStack.swift":1:1 parent @loadFromStack : $@convention(thin) (Int) -> Builtin.Int32 }
+// CHECK: sil_scope [[STACKSCOPE2:[0-9]+]] { loc "./loadFromStack.swift":1:1 parent [[STACKSCOPE1]] }
+
+// CHECK-LABEL: sil [ossa] @loadFromStack : $@convention(thin) (Int) -> Builtin.Int32 {
+// CHECK: bb0(%0 : $Int):
+// CHECK:   [[STACK:%.*]] = alloc_stack $Int
+// CHECK:   struct_element_addr [[STACK]] : $*Int, #Int._value
+// CHECK:   load [trivial] %{{.*}} : $*Builtin.Int32, loc "./loadFromStack.swift":2:1, scope [[STACKSCOPE1]]
+// CHECK:   debug_value [[STACK]] : $*Int, let, name "sVar", loc "./loadFromStack.swift":3:1, scope [[STACKSCOPE2]]
+// CHECK-LABEL: } // end sil function 'loadFromStack'
+sil [ossa] @loadFromStack : $@convention(thin) (Int) -> Builtin.Int32 {
+bb0(%0 : $Int):
+  debug_value %0 : $Int, let, name "s", argno 1
+  %2 = alloc_stack $Int
+  %3 = alloc_stack $Int
+  store %0 to [trivial] %3 : $*Int
+  %5 = function_ref @storeToStack : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> @out τ_0_0
+  %6 = apply %5<Int>(%2, %3) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> @out τ_0_0
+  dealloc_stack %3 : $*Int
+  %8 = load [trivial] %2 : $*Int, loc "./loadFromStack.swift":2:1, scope 20
+  debug_value %8 : $Int, let, name "sVar", loc "./loadFromStack.swift":3:1, scope 21
+  dealloc_stack %2 : $*Int
+  %11 = struct_extract %8 : $Int, #Int._value
+  return %11 : $Builtin.Int32
+}