At -Onone preserve debug info after splitting loads

Load splitting converts an aggregate load into a set of subobject
loads. This is required at -Onone for exclusivity diagnostics.

We cannot preserve the original debug information by redirecting debug
info to the memory address, because that might result in incorrect
debug values if the memory is reused.

Before this fix, we "conservatively" drop debug info in those
cases. This fix preserves full debug info by keeping the original
aggregate load intact alongside the new subobject loads. To avoid
exclusivity violations, it create a new unsafe access scope for the
old load.

Fixes LLDB missing variables in certain case #62241

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -1907,14 +1907,16 @@ class AccessUseDefChainCloner
 /// Clone all projections and casts on the access use-def chain until the
 /// checkBase predicate returns a valid base.
 ///
+/// Returns the cloned value equivalent to \p addr.
+///
 /// This will not clone ref_element_addr or ref_tail_addr because those aren't
 /// part of the access chain.
 ///
 /// CheckBase is a unary predicate that takes the next source address and either
 /// returns a valid SILValue to use as the base of the cloned access path, or an
 /// invalid SILValue to continue cloning.
 ///
-/// CheckBase must return a valid SILValue either before attempting to clone the
+/// CheckBase must return a valid SILValue before attempting to clone the
 /// access base. The most basic valid predicate is:
 ///
 ///    auto checkBase = [&](SILValue srcAddr) {
@@ -1929,6 +1931,8 @@ SILValue cloneUseDefChain(SILValue addr, SILInstruction *insertionPoint,
 
 /// Analog to cloneUseDefChain to check validity. begin_borrow and
 /// mark_dependence currently cannot be cloned.
+///
+/// Returns the cloned value equivalent to \p addr.
 template <typename CheckBase>
 bool canCloneUseDefChain(SILValue addr, CheckBase checkBase) {
   return AccessUseDefChainCloner<CheckBase>(checkBase, nullptr)

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

@@ -21,6 +21,7 @@
 #include "swift/SILOptimizer/Utils/CanonicalizeInstruction.h"
 #include "swift/SIL/DebugUtils.h"
 #include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/MemAccessUtils.h"
 #include "swift/SIL/Projection.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SIL/SILFunction.h"
@@ -137,6 +138,77 @@ static void replaceUsesOfExtract(SingleValueInstruction *extract,
   extract->replaceAllUsesWith(loadedVal);
 }
 
+// If \p loadInst has an debug uses, then move it into a separate unsafe access
+// scope. This hides it from the exclusivity checker.
+//
+// If \p loadInst was successfully hidden, then this returns the next
+// instruction following \p loadInst and following any newly inserted
+// instructions. Otherwise this returns nullptr. Returning nullptr is a signal
+// to delete \p loadInst.
+//
+// Before:
+//
+//   %a = begin_access %0 [read] [unknown]
+//   %proj = some_projections %a
+//   %whole = load %proj             // <-- loadInst
+//   %field = struct_element_addr %proj, #field
+//   %part = load %field
+//
+// After:
+//
+//   %a = begin_access %0 [read] [unknown]
+//   %proj = some_projections %a
+//   %a2 = begin_access %0 [read] [unsafe] // NEW
+//   %proj2 = some_projections %a          // CLONED
+//   %whole = load %proj2                  // <-- loadInst
+//   end_access %a2                        // NEW
+//   %field = struct_element_addr %proj, #field
+//   %part = load %field
+//
+static SILInstruction *
+moveLoadToUnsafeAccessScope(LoadInst *loadInst,
+                            CanonicalizeInstruction &pass) {
+  if (llvm::none_of(loadInst->getUses(), [](Operand *use) {
+        return use->getUser()->isDebugInstruction();
+      })) {
+    return nullptr;
+  }
+  SILValue accessScope = getAccessScope(loadInst->getOperand());
+  auto *access = dyn_cast<BeginAccessInst>(accessScope);
+  if (access && access->getEnforcement() == SILAccessEnforcement::Unsafe)
+    return nullptr;
+
+  auto checkBaseAddress = [=](SILValue addr) {
+    if (addr != accessScope)
+      return SILValue();
+
+    // the base of the new unsafe scope
+    if (access)
+      return access->getOperand();
+
+    return accessScope;
+  };
+
+  if (!canCloneUseDefChain(loadInst->getOperand(), checkBaseAddress))
+    return nullptr;
+
+  SILValue newBase =
+      cloneUseDefChain(loadInst->getOperand(), loadInst, checkBaseAddress);
+
+  auto *beginUnsafe = SILBuilderWithScope(loadInst).createBeginAccess(
+      loadInst->getLoc(), newBase, SILAccessKind::Read,
+      SILAccessEnforcement::Unsafe, true, false);
+  loadInst->setOperand(beginUnsafe);
+  auto nextInst = loadInst->getNextInstruction();
+  auto *endUnsafe = SILBuilderWithScope(nextInst).createEndAccess(
+      loadInst->getLoc(), beginUnsafe, false);
+
+  pass.notifyNewInstruction(beginUnsafe);
+  pass.notifyNewInstruction(endUnsafe);
+
+  return nextInst;
+}
+
 // Given a load with multiple struct_extracts/tuple_extracts and no other uses,
 // canonicalize the load into several (struct_element_addr (load)) pairs.
 //
@@ -301,16 +373,9 @@ 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.
+    // FIXME: 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 it caused on LLVM back-end crash.
     if (!pass.preserveDebugInfo && EnableLoadSplittingDebugInfo) {
       createDebugFragments(*loadInst, proj, lastNewLoad->getLoadInst());
     }
@@ -340,13 +405,23 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
   for (auto *borrow : borrows)
     nextII = killInstAndIncidentalUses(borrow, nextII, pass);
 
+  // When pass.preserveDebugInfo is true, keep the original load so that debug
+  // info refers to the loaded value, rather than a memory location which may
+  // not be reused. Move the wide load out of its current access scope and into
+  // an "unknown" access scope, which won't be enforced as an exclusivity
+  // violation.
+  if (pass.preserveDebugInfo) {
+    if (auto *regularLoad = dyn_cast<LoadInst>(loadInst.getLoadInst())) {
+      if (auto *nextInst = moveLoadToUnsafeAccessScope(regularLoad, pass))
+        return nextInst->getIterator();
+    }
+  }
   // Erase the old load.
   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.
+  // debug_value.
   while (nextII != loadInst->getParent()->end()
          && nextII->isDebugInstruction()) {
     ++nextII;

test/SILOptimizer/silgen_cleanup.sil

@@ -362,3 +362,33 @@ bb9(%0 : @owned $Klass):
   destroy_value %0 : $Klass
   return %v : $Builtin.Int64
 }
+
+// debug_value must be preserved after splitting loads
+
+struct IntWrapper {
+  var _value : Int
+}
+
+sil_scope 5 { loc "./test.swift":3:6 parent @testSplitLoadDebug : $@convention(thin) (@in_guaranteed IntWrapper) -> Builtin.Int32 }
+sil_scope 6 { loc "./test.swift":3:10 parent 5 }
+
+// CHECK-LABEL: sil hidden [ossa] @testSplitLoadDebug : $@convention(thin) (@in_guaranteed IntWrapper) -> Builtin.Int32 {
+// CHECK: bb0(%0 : $*IntWrapper):
+// CHECKDEB:   [[A1:%.*]] = struct_element_addr %0 : $*IntWrapper, #IntWrapper._value
+// CHECKDEB:   [[A2:%.*]] = struct_element_addr [[A1]] : $*Int, #Int._value
+// CHECKDEB:   [[SPLIT:%.*]] = load [trivial] %2 : $*Builtin.Int32
+// CHECKDEB:   [[A3:%.*]] = struct_element_addr %0 : $*IntWrapper, #IntWrapper._value
+// CHECKDEB:   [[A4:%.*]] = begin_access [read] [unsafe] [no_nested_conflict] [[A3]] : $*Int
+// CHECKDEB:   [[OLD:%.*]] = load [trivial] [[A4]] : $*Int
+// CHECKDEB:   end_access [[A4]] : $*Int
+// CHECKDEB:   debug_value [[OLD]] : $Int, let, name "flag"
+// CHECKDEB:   return [[SPLIT]] : $Builtin.Int32
+// CHECK-LABEL: } // end sil function 'testSplitLoadDebug'
+sil hidden [ossa] @testSplitLoadDebug : $@convention(thin) (@in_guaranteed IntWrapper) -> Builtin.Int32 {
+bb0(%0: $*IntWrapper):
+  %1 = struct_element_addr %0 : $*IntWrapper, #IntWrapper._value
+  %2 = load [trivial] %1 : $*Int
+  debug_value %2 : $Int, let, name "flag", loc "./test.swift":4:7, scope 6
+  %4 = struct_extract %2 : $Int, #Int._value
+  return %4 : $Builtin.Int32
+}