[SSADestroyHoisting] Don't fold over trivial use.

nate-chandler · nate-chandler · commit 4dec48f9a6ae · 2023-01-05T15:18:36.000-08:00
Given an aggregate addr `%agg` with trivial subobject addr `%triv` and
nontrivial subobject addr `%sub` which `%triv` is projected from,
```
  Aggregate   &lt;- %agg
    Subobject &lt;- %sub
      Trivial &lt;- %triv
      ...
    ...
```

after `%sub` is destroyed, `%triv` is no longer initialized.  As a
result, it's not valid to fold a destroy_addr of %agg into a sequence of
`load [copy]`s and `copy_addr`s if there's an access to `%triv` after
the `load [copy]`/`copy_addr` of `%sub` (or some intermediate
subobject).

In other words, transforming
```
  copy_addr %sub
  load [trivial] %triv
  destroy_addr %agg
```
into
```
  copy_addr [take] %sub
  load [trivial] %triv
```
is invalid.

During destroy_addr folding, prevent that from happening by keeping
track of the trivial fields that have already been visited.  If a
trivial field is seen more than once, then bail on folding.  This is
the same as what is done for non-trivial fields except that there's no
requirement that all trivial fields be destroyed.
diff --git a/lib/SILOptimizer/Transforms/SSADestroyHoisting.cpp b/lib/SILOptimizer/Transforms/SSADestroyHoisting.cpp
@@ -441,6 +441,7 @@ class HoistDestroys {
                            SmallVectorImpl<LoadInst *> &loads,
                            SmallVectorImpl<CopyAddrInst *> &copies,
                            SmallPtrSetImpl<AccessPath::PathNode> &leaves,
+                           SmallPtrSetImpl<AccessPath::PathNode> &trivialLeaves,
                            const AccessStorage &storage,
                            const DeinitBarriers &deinitBarriers);
 
@@ -568,6 +569,25 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
   // destroy_addr in order for folding to occur.
   llvm::SmallPtrSet<AccessPath::PathNode, 16> leaves;
 
+  // The trivial storage leaves of the root storage.  They needn't be destroyed
+  // in the sequence prior to the destroy_addr, but their uses may obstruct
+  // folding.  For example,  given an %object and %triv a trivial subobject
+  //
+  //     load [copy] %object
+  //     load [trivial] %triv
+  //     destroy_addr %object
+  //
+  // it isn't legal to fold the destroy_addr into the load of %object like
+  //
+  //     load [take] %object
+  //     load [trivial] %triv
+  //
+  // because the memory location %triv is no longer valid.  In general, it would
+  // be fine to support folding over accesses of trivial subobjects so long as
+  // they occur prior to the access to some nontrivial subobject that contains
+  // it.
+  SmallPtrSet<AccessPath::PathNode, 16> trivialLeaves;
+
   visitProductLeafAccessPathNodes(storageRoot, typeExpansionContext, module,
                                   [&](AccessPath::PathNode node, SILType ty) {
                                     if (ty.isTrivial(*function))
@@ -577,8 +597,8 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
 
   for (auto *instruction = barrier; instruction != nullptr;
        instruction = instruction->getPreviousInstruction()) {
-    if (checkFoldingBarrier(instruction, loads, copies, leaves, storage,
-                            deinitBarriers))
+    if (checkFoldingBarrier(instruction, loads, copies, leaves, trivialLeaves,
+                            storage, deinitBarriers))
       return false;
 
     // If we have load [copy]s or copy_addrs of projections out of the root
@@ -672,8 +692,9 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
 bool HoistDestroys::checkFoldingBarrier(
     SILInstruction *instruction, SmallVectorImpl<LoadInst *> &loads,
     SmallVectorImpl<CopyAddrInst *> &copies,
-    SmallPtrSetImpl<AccessPath::PathNode> &leaves, const AccessStorage &storage,
-    const DeinitBarriers &deinitBarriers) {
+    SmallPtrSetImpl<AccessPath::PathNode> &leaves,
+    SmallPtrSetImpl<AccessPath::PathNode> &trivialLeaves,
+    const AccessStorage &storage, const DeinitBarriers &deinitBarriers) {
   // The address of a projection out of the root storage which would be
   // folded if folding is possible.
   //
@@ -720,14 +741,18 @@ bool HoistDestroys::checkFoldingBarrier(
     // Find its nontrivial product leaves and remove them from the set of
     // leaves of the root storage which we're wating to see.
     bool alreadySawLeaf = false;
-    visitProductLeafAccessPathNodes(address, typeExpansionContext, module,
-                                    [&](AccessPath::PathNode node, SILType ty) {
-                                      if (ty.isTrivial(*function))
-                                        return;
-                                      bool erased = leaves.erase(node);
-                                      alreadySawLeaf =
-                                          alreadySawLeaf || !erased;
-                                    });
+    bool alreadySawTrivialSubleaf = false;
+    visitProductLeafAccessPathNodes(
+        address, typeExpansionContext, module,
+        [&](AccessPath::PathNode node, SILType ty) {
+          if (ty.isTrivial(*function)) {
+            bool inserted = !trivialLeaves.insert(node).second;
+            alreadySawTrivialSubleaf = alreadySawTrivialSubleaf || inserted;
+            return;
+          }
+          bool erased = leaves.erase(node);
+          alreadySawLeaf = alreadySawLeaf || !erased;
+        });
     if (alreadySawLeaf) {
       // We saw this non-trivial product leaf already.  That means there are
       // multiple load [copy]s or copy_addrs of at least one product leaf
@@ -736,6 +761,11 @@ bool HoistDestroys::checkFoldingBarrier(
       // Give up on folding.
       return true;
     }
+    if (alreadySawTrivialSubleaf) {
+      // We saw this trivial leaf already.  That means there was some later
+      // load [copy] or copy_addr of it.  Give up on folding.
+      return true;
+    }
   } else if (deinitBarriers.isBarrier(instruction)) {
     // We didn't find an instruction that was both
     // - relevant (i.e. a copy_addr or a load [take])
diff --git a/test/SILOptimizer/hoist_destroy_addr.sil b/test/SILOptimizer/hoist_destroy_addr.sil
@@ -60,6 +60,11 @@ struct I {
   var value: Builtin.Int64
 }
 
+struct Slice {
+    var _startIndex: I
+    var _guts: AnyObject
+}
+
 typealias TXXI = (X, X, I)
 
 struct SXXI {
@@ -1073,6 +1078,61 @@ entry(%instance : @owned $AnyObject):
     return %cast : $X
 }
 
+// Don't fold a destroy_addr into a copy_addr of the whole aggregate if there
+// is an access to some trivial subobject afterwards.
+// CHECK-LABEL: sil [ossa] @nofold_destroy_addr_into_whole_with_later_trivial_subobject_use : {{.*}} {
+// CHECK:         destroy_addr
+// CHECK-LABEL: } // end sil function 'nofold_destroy_addr_into_whole_with_later_trivial_subobject_use'
+sil [ossa] @nofold_destroy_addr_into_whole_with_later_trivial_subobject_use : $@convention(thin) (@in Slice) -> (@out Slice) {
+bb0(%out : $*Slice, %4 : $*Slice):
+  copy_addr %4 to [init] %out : $*Slice
+  %8 = alloc_stack $I
+  %9 = struct_element_addr %4 : $*Slice, #Slice._startIndex
+  copy_addr %9 to [init] %8 : $*I
+  destroy_addr %4 : $*Slice
+  dealloc_stack %8 : $*I
+  %19 = tuple ()
+  return %19 : $()
+}
+
+// Like fold_struct_enclosing_tuples_leaf_fields_mixed_insts but with an access
+// of a trivial field afterwards.
+// CHECK-LABEL: sil [ossa] @nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject : {{.*}} {
+// CHECK:         load [trivial]
+// CHECK:         destroy_addr {{%[^,]+}} : $*STXXITXXII
+// CHECK-LABEL: } // end sil function 'nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject'
+sil [ossa] @nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject : $@convention(thin) (@owned STXXITXXII) -> @owned (X, X) {
+entry(%instance : @owned $STXXITXXII):
+    %addr = alloc_stack $STXXITXXII
+    %txxi_1_alone = alloc_stack $X
+    %txxi_2_alone = alloc_stack $X
+    store %instance to [init] %addr : $*STXXITXXII
+    %txxi_1_addr = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.txxi_1
+    %txxi_2_addr = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.txxi_2
+    %txxi_1_x_0_addr = tuple_element_addr %txxi_1_addr : $*(X, X, I), 0
+    %txxi_1_x_1_addr = tuple_element_addr %txxi_1_addr : $*(X, X, I), 1
+    %txxi_2_x_0_addr = tuple_element_addr %txxi_2_addr : $*(X, X, I), 0
+    %txxi_2_x_1_addr = tuple_element_addr %txxi_2_addr : $*(X, X, I), 1
+    %trivial = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.i
+
+    copy_addr %txxi_1_x_0_addr to [init] %txxi_1_alone : $*X
+    %x1x1 = load [copy] %txxi_1_x_1_addr : $*X
+    copy_addr %txxi_2_x_0_addr to [init] %txxi_2_alone : $*X
+    %x2x1 = load [copy] %txxi_2_x_1_addr : $*X
+    %i = load [trivial] %trivial : $*I
+    destroy_addr %addr : $*STXXITXXII
+    %barrier = function_ref @unknown : $@convention(thin) () -> ()
+    apply %barrier() : $@convention(thin) () -> ()
+
+    destroy_addr %txxi_1_alone : $*X
+    destroy_addr %txxi_2_alone : $*X
+    dealloc_stack %txxi_2_alone : $*X
+    dealloc_stack %txxi_1_alone : $*X
+    dealloc_stack %addr : $*STXXITXXII
+    %retval = tuple (%x1x1 : $X, %x2x1 : $X)
+    return %retval : $(X, X)
+}
+
 // CHECK-LABEL: sil [ossa] @hoist_over_apply_of_non_barrier_fn : {{.*}} {
 // CHECK:         destroy_addr
 // CHECK:         apply
