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

At -Onone preserve debug info after splitting loads

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
+}