CanonicalizeInstruction - FIXME and option to preserve debug info

Debug information needs to be preserved at -Onone.

And it should be preserved to a reasonable extend at -O, not
completely thrown away.

This transformation does neither.

The -Onone case is more important. The FIXME explains how that could
be done.

I'm including an example of how to handle the -O case, currently under
a a flag, -sil-load-splitting-debug-info.

This -O support uses debug fragments, which currently crashes when
building SwiftPM's PackageModel module in LLVM's 'X86 Assembly
Printer', llvm::DwarfExpression::addFragmentOffset.

We don't appear to have any verification of debug fragment in either
SIL or LLVM that would catch this. And I haven't been able to reduce
the problem to a small test case.

Author: atrick

lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp

@@ -36,6 +36,10 @@ using namespace swift;
 // Tracing within the implementation can also be activiated 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() {}
 
@@ -243,6 +247,9 @@ splitAggregateLoad(LoadOperation loadInst, CanonicalizeInstruction &pass) {
   //   }
   // }
   //
+  // Also, avoid degrading debug info unless it is necessary for exclusivity
+  // diagnostics.
+  //
   // TODO: This logic subtly anticipates SILGen behavior. In the future, change
   // SILGen to avoid emitting the full load and never delete loads in Raw SIL.
   if (projections.empty() && loadInst->getModule().getStage() == SILStage::Raw)
@@ -294,14 +301,27 @@ 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.
         assert(loadInst.getOwnershipQualifier() ==
                LoadOwnershipQualifier::Copy);
         for (SILInstruction *destroy : lifetimeEndingInsts) {
           auto *newInst = SILBuilderWithScope(destroy).createDestroyValue(
-              destroy->getLoc(), **lastNewLoad);
+              destroy->getLoc(), lastNewLoad->getLoadInst());
           pass.notifyNewInstruction(newInst);
         }
       }
@@ -324,6 +344,14 @@ 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.
+  while (nextII != loadInst->getParent()->end()
+         && nextII->isDebugInstruction()) {
+    ++nextII;
+  }
+  deleteAllDebugUses(*loadInst, pass.getCallbacks());
   return killInstAndIncidentalUses(*loadInst, nextII, pass);
 }
 
@@ -533,7 +561,6 @@ CanonicalizeInstruction::canonicalize(SILInstruction *inst) {
   if (auto li = LoadOperation(inst)) {
     return splitAggregateLoad(li, *this);
   }
-
   if (auto *storeInst = dyn_cast<StoreInst>(inst)) {
     return broadenSingleElementStores(storeInst, *this);
   }

test/SILOptimizer/silgen_cleanup.sil

@@ -26,6 +26,12 @@ struct UInt8 {
   var _value : Builtin.Int8
 }
 
+// =============================================================================
+// Test splitAggregateLoad
+//
+// Required for exclusivity diagnostics that are consistent across -Onone / -O
+// =============================================================================
+
 // CHECK-LABEL: sil [ossa] @struct_extract_load_to_load_struct_element_addr
 // CHECK: bb0([[IN:%[0-9]+]] : $*UInt8):
 // CHECK-NEXT:  [[IN_GEP:%[0-9]+]] = struct_element_addr [[IN]] : $*UInt8, #UInt8._value
@@ -187,6 +193,36 @@ bb0(%0 : $*X1):
   return %result : $(Int, Builtin.NativeObject, Builtin.NativeObject)
 }
 
+sil private [ossa] @testLoadWithDebugInfo : $@convention(thin) (@inout_aliasable X1) -> (Int, @owned Builtin.NativeObject, @owned Builtin.NativeObject) {
+bb0(%0 : $*X1):
+  %access = begin_access [read] [unknown] %0 : $*X1
+  %load = load [copy] %access : $*X1
+  debug_value %load : $Int, let, name "x1"
+  end_access %access : $*X1
+
+  %borrowa = begin_borrow %load : $X1
+  %a = struct_extract %borrowa : $X1, #X1.a
+  end_borrow %borrowa : $X1
+
+  %borrow1 = begin_borrow %load : $X1
+  %o1 = struct_extract %borrow1 : $X1, #X1.obj1
+  %copy1 = copy_value %o1 : $Builtin.NativeObject
+  end_borrow %borrow1 : $X1
+
+  %borrow2 = begin_borrow %load : $X1
+  %o2 = struct_extract %borrow2 : $X1, #X1.obj2
+  %copy2 = copy_value %o2 : $Builtin.NativeObject
+  end_borrow %borrow2 : $X1
+
+  destroy_value %load : $X1
+
+  %result = tuple (%a : $Int, %copy1 : $Builtin.NativeObject, %copy2 : $Builtin.NativeObject)
+  return %result : $(Int, Builtin.NativeObject, Builtin.NativeObject)
+}
+
+// =============================================================================
+// Test broadenSingleElementStores
+// =============================================================================
 
 struct X2 {
   @_hasStorage @_hasInitialValue var obj: Builtin.NativeObject { get set }
@@ -217,6 +253,10 @@ bb0(%0 : $*X3, %1 : @guaranteed $Builtin.NativeObject):
   return %12 : $()
 }
 
+// =============================================================================
+// Test simple copy and borrow elimination
+// =============================================================================
+
 // We used to hit a memory error on this test.
 //
 // CHECK-LABEL: sil [ossa] @testDestructureTupleNoCrash : $@convention(thin) (@owned (Builtin.NativeObject, Builtin.NativeObject)) -> () {