Merge pull request #63240 from atrick/5.8-revert-debugfix

[5.8] Revert "At -Onone preserve debug info after splitting loads"

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -1909,16 +1909,14 @@ 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 before attempting to clone the
+/// CheckBase must return a valid SILValue either before attempting to clone the
 /// access base. The most basic valid predicate is:
 ///
 ///    auto checkBase = [&](SILValue srcAddr) {
@@ -1933,8 +1931,6 @@ 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,7 +21,6 @@
 #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"
@@ -138,77 +137,6 @@ 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.
 //
@@ -373,9 +301,16 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
     }
     pass.notifyNewInstruction(**lastNewLoad);
 
-    // 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.
+    // 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());
     }
@@ -405,23 +340,13 @@ 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.
+  // debug_value. A soon-to-be merged commit migrates CanonicalizeInstruction to
+  // use InstructionDeleter.
   while (nextII != loadInst->getParent()->end()
          && nextII->isDebugInstruction()) {
     ++nextII;

test/SILOptimizer/silgen_cleanup.sil

@@ -362,33 +362,3 @@ 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
-}