Merge pull request #62609 from nate-chandler/opaque-values/2/20221214

[AddressLowering] Copy for out-of-range stores.

Author: swift-ci

lib/SIL/Verifier/SILOwnershipVerifier.cpp

@@ -683,7 +683,7 @@ bool SILValueOwnershipChecker::checkUses() {
   // outlive the current function always.
   //
   // In the case of a yielded guaranteed value, we need to validate that all
-  // regular uses of the value are within the co
+  // regular uses of the value are within the coroutine.
   if (lifetimeEndingUsers.empty()) {
     if (checkValueWithoutLifetimeEndingUses(regularUsers))
       return false;

lib/SILOptimizer/Mandatory/AddressLowering.cpp

@@ -347,6 +347,27 @@ static bool isStoreCopy(SILValue value) {
   if (!copyInst->hasOneUse())
     return false;
 
+  auto *user = value->getSingleUse()->getUser();
+  auto *storeInst = dyn_cast<StoreInst>(user);
+  if (!storeInst)
+    return false;
+
+  SSAPrunedLiveness liveness;
+  auto isStoreOutOfRange = [&liveness, storeInst](SILValue root) {
+    liveness.initializeDef(root);
+    auto summary = liveness.computeSimple();
+    if (summary.addressUseKind != AddressUseKind::NonEscaping) {
+      return true;
+    }
+    if (summary.innerBorrowKind != InnerBorrowKind::Contained) {
+      return true;
+    }
+    if (!liveness.isWithinBoundary(storeInst)) {
+      return true;
+    }
+    return false;
+  };
+
   auto source = copyInst->getOperand();
   if (source->getOwnershipKind() == OwnershipKind::Guaranteed) {
     // [in_guaranteed_begin_apply_results] If any root of the source is a
@@ -356,19 +377,30 @@ static bool isStoreCopy(SILValue value) {
     SmallVector<SILValue, 4> roots;
     findGuaranteedReferenceRoots(source, /*lookThroughNestedBorrows=*/true,
                                  roots);
-    if (llvm::any_of(roots, [](SILValue root) {
-      // Handle forwarding phis conservatively rather than recursing.
-      if (SILArgument::asPhi(root) && !BorrowedValue(root))
-        return true;
-
-      return isa<BeginApplyInst>(root->getDefiningInstruction());
-    })) {
+    // TODO: Rather than checking whether the store is out of range of any
+    // guaranteed root's SSAPrunedLiveness, instead check whether it is out of
+    // range of ExtendedLiveness of the borrow introducers:
+    // - visit borrow introducers via visitBorrowIntroducers
+    // - call ExtendedLiveness.compute on each borrow introducer
+    if (llvm::any_of(roots, [&](SILValue root) {
+          // Handle forwarding phis conservatively rather than recursing.
+          if (SILArgument::asPhi(root) && !BorrowedValue(root))
+            return true;
+
+          if (isa<BeginApplyInst>(root->getDefiningInstruction())) {
+            return true;
+          }
+          return isStoreOutOfRange(root);
+        })) {
+      return false;
+    }
+  } else if (source->getOwnershipKind() == OwnershipKind::Owned) {
+    if (isStoreOutOfRange(source)) {
       return false;
     }
   }
 
-  auto *user = value->getSingleUse()->getUser();
-  return isa<StoreInst>(user);
+  return true;
 }
 
 void ValueStorageMap::insertValue(SILValue value, SILValue storageAddress) {
@@ -2280,9 +2312,9 @@ void ApplyRewriter::convertBeginApplyWithOpaqueYield() {
                                      info.isConsumed() ? IsTake : IsNotTake,
                                      IsInitialization);
       } else {
-        // [in_guaranteed_begin_apply_results] Because OSSA ensure that all uses
-        // of a guaranteed value produced by a begin_apply are used within the
-        // coroutine's range, AddressLowering will not introduce uses of
+        // [in_guaranteed_begin_apply_results] Because OSSA ensures that all
+        // uses of a guaranteed value produced by a begin_apply are used within
+        // the coroutine's range, AddressLowering will not introduce uses of
         // invalid memory by rewriting the uses of a yielded guaranteed opaque
         // value as uses of yielded guaranteed storage.  However, it must
         // allocate storage for copies of [projections of] such values.
@@ -2994,12 +3026,12 @@ class UseRewriter : SILInstructionVisitor<UseRewriter> {
     switch (bi->getBuiltinKind().value_or(BuiltinValueKind::None)) {
     case BuiltinValueKind::ResumeNonThrowingContinuationReturning: {
       SILValue opAddr = addrMat.materializeAddress(use->get());
-      bi->setOperand(1, opAddr);
+      bi->setOperand(use->getOperandNumber(), opAddr);
       break;
     }
     case BuiltinValueKind::ResumeThrowingContinuationReturning: {
       SILValue opAddr = addrMat.materializeAddress(use->get());
-      bi->setOperand(1, opAddr);
+      bi->setOperand(use->getOperandNumber(), opAddr);
       break;
     }
     case BuiltinValueKind::Copy: {
@@ -3768,7 +3800,7 @@ static void rewriteFunction(AddressLoweringState &pass) {
       originalUses.insert(oper);
       UseRewriter::rewriteUse(oper, pass);
     }
-    // Rewrite every new uses that was added.
+    // Rewrite every new use that was added.
     uses.clear();
     for (auto *use : valueDef->getUses()) {
       if (originalUses.contains(use))

test/SILOptimizer/address_lowering.sil

@@ -1858,6 +1858,72 @@ entry:
   return %retval : $()
 }
 
+// CHECK-LABEL: sil [ossa] @testCopyValue1StoreExtractAfterDestroyAggregate : {{.*}} {
+// CHECK:       {{bb[0-9]+}}([[ADDR:%[^,]+]] :
+// CHECK:         [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $Pair<T>
+// CHECK:         [[COPY_ADDR:%[^,]+]] = alloc_stack $T
+// CHECK:         apply undef<T>([[INSTANCE_ADDR]])
+// CHECK:         [[X_ADDR:%[^,]+]] = struct_element_addr [[INSTANCE_ADDR]]{{.*}}, #Pair.x
+// CHECK:         copy_addr [[X_ADDR]] to [init] [[COPY_ADDR]]
+// CHECK:         destroy_addr [[INSTANCE_ADDR]]
+// CHECK:         copy_addr [take] [[COPY_ADDR]] to [[ADDR]]
+// CHECK-LABEL: } // end sil function 'testCopyValue1StoreExtractAfterDestroyAggregate'
+sil [ossa] @testCopyValue1StoreExtractAfterDestroyAggregate : $@convention(thin) <T> (@inout T) -> () {
+entry(%addr : $*T):
+    %instance = apply undef<T>() : $@convention(thin) <Tee> () -> @out Pair<Tee>
+    %lifetime = begin_borrow %instance : $Pair<T>
+    %x = struct_extract %lifetime : $Pair<T>, #Pair.x
+    %copy = copy_value %x : $T
+    end_borrow %lifetime : $Pair<T>
+    destroy_value %instance : $Pair<T>
+    store %copy to [assign] %addr : $*T
+    %retval = tuple ()
+    return %retval : $()
+}
+
+// CHECK-LABEL: sil [ossa] @testCopyValue2StoreCopyAfterDestroy : {{.*}} {
+// CHECK:       {{bb[0-9]+}}([[ADDR:%[^,]+]] :
+// CHECK:         [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $T
+// CHECK:         [[COPY_ADDR:%[^,]+]] = alloc_stack $T
+// CHECK:         apply undef<T>([[INSTANCE_ADDR]])
+// CHECK:         copy_addr [[INSTANCE_ADDR]] to [init] [[COPY_ADDR]]
+// CHECK:         destroy_addr [[INSTANCE_ADDR]]
+// CHECK:         copy_addr [take] [[COPY_ADDR]] to [[ADDR]]
+// CHECK-LABEL: } // end sil function 'testCopyValue2StoreCopyAfterDestroy'
+sil [ossa] @testCopyValue2StoreCopyAfterDestroy : $@convention(thin) <T> (@inout T) -> () {
+entry(%addr : $*T):
+    %instance = apply undef<T>() : $@convention(thin) <Tee> () -> @out Tee
+    %copy = copy_value %instance : $T
+    destroy_value %instance : $T
+    store %copy to [assign] %addr : $*T
+    %retval = tuple ()
+    return %retval : $()
+}
+
+// CHECK-LABEL: sil [ossa] @testCopyValue3StoreTupleDestructureFieldAfterDestroy : $@convention(thin) <T> (@inout T) -> () {
+// CHECK:       {{bb[0-9]+}}([[ADDR:%[^,]+]] :
+// CHECK:         [[INSTANCE_ADDR:%[^,]+]] = alloc_stack $(T, T)
+// CHECK:         [[COPY_ADDR:%[^,]+]] = alloc_stack $T
+// CHECK:         apply undef<T>([[INSTANCE_ADDR]]) : $@convention(thin) <τ_0_0> () -> @out (τ_0_0, τ_0_0)
+// CHECK:         [[ONE_ADDR:%[^,]+]] = tuple_element_addr [[INSTANCE_ADDR]]{{.*}}, 0
+// CHECK:         [[TWO_ADDR:%[^,]+]] = tuple_element_addr [[INSTANCE_ADDR]]{{.*}}, 1
+// CHECK:         copy_addr [[ONE_ADDR]] to [init] [[COPY_ADDR]]
+// CHECK:         destroy_addr [[ONE_ADDR]]
+// CHECK:         destroy_addr [[TWO_ADDR]]
+// CHECK:         copy_addr [take] [[COPY_ADDR]] to [[ADDR]]
+// CHECK-LABEL: } // end sil function 'testCopyValue3StoreTupleDestructureFieldAfterDestroy'
+sil [ossa] @testCopyValue3StoreTupleDestructureFieldAfterDestroy : $@convention(thin) <T> (@inout T) -> () {
+entry(%addr : $*T):
+    %instance = apply undef<T>() : $@convention(thin) <Tee> () -> @out (Tee, Tee)
+    (%one, %two) = destructure_tuple %instance : $(T, T)
+    %copy = copy_value %one : $T
+    destroy_value %one : $T
+    destroy_value %two : $T
+    store %copy to [assign] %addr : $*T
+    %retval = tuple ()
+    return %retval : $()
+}
+
 // CHECK-LABEL: sil hidden [ossa] @testOpaqueYield :
 // CHECK: bb0(%0 : @guaranteed $TestGeneric<T>):
 // CHECK:   [[REF:%.*]] = ref_element_addr %0 : $TestGeneric<T>, #TestGeneric.borrowedGeneric

test/SILOptimizer/opaque_values_Onone_stdlib.swift

@@ -41,7 +41,7 @@ func foo(@_noImplicitCopy _ x: __owned X) {
 // CHECK:       {{bb[0-9]+}}([[OUT_ADDR:%[^,]+]] : $*T, [[IN_ADDR:%[^,]+]] : $*T):
 // CHECK:         [[TMP_ADDR:%[^,]+]] = alloc_stack $T
 // CHECK:         copy_addr [[IN_ADDR]] to [init] [[TMP_ADDR]] : $*T
-// CHECK:         [[REGISTER_5:%[^,]+]] = builtin "copy"<T>([[OUT_ADDR]] : $*T, [[TMP_ADDR]] : $*T) : $()
+// CHECK:         builtin "copy"<T>([[OUT_ADDR]] : $*T, [[TMP_ADDR]] : $*T) : $()
 // CHECK:         dealloc_stack [[TMP_ADDR]] : $*T
 // CHECK:         return {{%[^,]+}} : $()
 // CHECK-LABEL: } // end sil function '_copy'