Merge pull request swiftlang#79084 from jckarter/addressor-projection-lifetime

SILGen: Materialize addressor bases for the formal access scope of the entire access.

Author: jckarter

lib/SILGen/RValue.cpp

@@ -409,10 +409,6 @@ static void verifyHelper(ArrayRef<ManagedValue> values,
   ValueOwnershipKind result = OwnershipKind::None;
   std::optional<bool> sameHaveCleanups;
   for (ManagedValue v : values) {
-    assert((!SGF || !v.getType().isLoadable(SGF.get()->F) ||
-            v.getType().isObject()) &&
-           "All loadable values in an RValue must be an object");
-
     ValueOwnershipKind kind = v.getOwnershipKind();
     if (kind == OwnershipKind::None)
       continue;

lib/SILGen/SILGenApply.cpp

@@ -7012,13 +7012,20 @@ bool AccessorBaseArgPreparer::shouldLoadBaseAddress() const {
   // base isn't a temporary.  We aren't allowed to pass aliased
   // memory to 'in', and we have pass at +1.
   case ParameterConvention::Indirect_In:
-  case ParameterConvention::Indirect_In_Guaranteed:
   case ParameterConvention::Indirect_In_CXX:
     // TODO: We shouldn't be able to get an lvalue here, but the AST
     // sometimes produces an inout base for non-mutating accessors.
     // rdar://problem/19782170
     // assert(!base.isLValue());
-    return base.isLValue() || base.isPlusZeroRValueOrTrivial();
+    return base.isLValue() || base.isPlusZeroRValueOrTrivial()
+      || !SGF.silConv.useLoweredAddresses();
+
+  case ParameterConvention::Indirect_In_Guaranteed:
+    // We can pass the memory we already have at +0. The memory referred to
+    // by the base should be already immutable unless it was lowered as an
+    // lvalue.
+    return base.isLValue()
+      || !SGF.silConv.useLoweredAddresses();
 
   // If the accessor wants the value directly, we definitely have to
   // load.
@@ -7077,7 +7084,7 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorAddressBaseArg() {
 
   // Otherwise, we have a value that we can forward without any additional
   // handling.
-  return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base));
+  return ArgumentSource(loc, RValue(SGF, {base}, baseFormalType));
 }
 
 ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() {

lib/SILGen/SILGenLValue.cpp

@@ -1405,10 +1405,25 @@ namespace {
       if (base) {
         // Borrow the base, because we may need it again to invoke other
         // accessors.
-        result.base = SGF.prepareAccessorBaseArg(loc,
-                                             base.formalAccessBorrow(SGF, loc),
-                                             BaseFormalType,
+        base = base.formalAccessBorrow(SGF, loc);
+        // If the base needs to be materialized, do so in
+        // the outer formal evaluation scope, since an addressor or
+        // other dependent value may want to point into the materialization.
+        auto &baseInfo = SGF.getConstantInfo(SGF.getTypeExpansionContext(),
                                              accessor);
+                                             
+        if (!baseInfo.FormalPattern.isForeign()) {
+          auto baseFnTy = baseInfo.SILFnType;
+          
+          if (baseFnTy->getSelfParameter().isFormalIndirect()
+              && base.getType().isObject()
+              && SGF.silConv.useLoweredAddresses()) {
+            base = base.formallyMaterialize(SGF, loc);
+          }
+        }
+        
+        result.base = SGF.prepareAccessorBaseArg(loc, base, BaseFormalType,
+                                                 accessor);
       }
 
       if (!Indices.isNull())
@@ -2086,16 +2101,12 @@ namespace {
              "offsetting l-value for modification without writeback scope");
 
       ManagedValue addr;
-      {
-        FormalEvaluationScope scope(SGF);
-
-        auto args =
-            std::move(*this).prepareAccessorArgs(SGF, loc, base, Accessor);
-        addr = SGF.emitAddressorAccessor(
-            loc, Accessor, Substitutions, std::move(args.base), IsSuper,
-            IsDirectAccessorUse, std::move(args.Indices),
-            SubstFieldType, IsOnSelfParameter);
-      }
+      auto args =
+          std::move(*this).prepareAccessorArgs(SGF, loc, base, Accessor);
+      addr = SGF.emitAddressorAccessor(
+          loc, Accessor, Substitutions, std::move(args.base), IsSuper,
+          IsDirectAccessorUse, std::move(args.Indices),
+          SubstFieldType, IsOnSelfParameter);
 
       // Enter an unsafe access scope for the access.
       addr =

lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp

@@ -475,11 +475,11 @@ void MoveOnlyObjectCheckerPImpl::check(
       if (markedInst->getCheckKind() ==
           MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign) {
         if (auto *cvi = dyn_cast<CopyValueInst>(markedInst->getOperand())) {
-          SingleValueInstruction *i = cvi;
+          auto replacement = cvi->getOperand();
+          auto orig = replacement;
           if (auto *copyToMoveOnly =
-                  dyn_cast<CopyableToMoveOnlyWrapperValueInst>(
-                      cvi->getOperand())) {
-            i = copyToMoveOnly;
+                  dyn_cast<CopyableToMoveOnlyWrapperValueInst>(orig)) {
+            orig = copyToMoveOnly->getOperand();
           }
 
           // TODO: Instead of pattern matching specific code generation patterns,
@@ -492,14 +492,14 @@ void MoveOnlyObjectCheckerPImpl::check(
           // bb(%arg : @guaranteed $Type):
           //   %copy = copy_value %arg
           //   %mark = mark_unresolved_non_copyable_value [no_consume_or_assign] %copy
-          if (auto *arg = dyn_cast<SILArgument>(i->getOperand(0))) {
+          if (auto *arg = dyn_cast<SILArgument>(orig)) {
             if (arg->getOwnershipKind() == OwnershipKind::Guaranteed) {
               for (auto *use : markedInst->getConsumingUses()) {
                 destroys.push_back(cast<DestroyValueInst>(use->getUser()));
               }
               while (!destroys.empty())
                 destroys.pop_back_val()->eraseFromParent();
-              markedInst->replaceAllUsesWith(arg);
+              markedInst->replaceAllUsesWith(replacement);
               markedInst->eraseFromParent();
               cvi->eraseFromParent();
               continue;
@@ -511,13 +511,13 @@ void MoveOnlyObjectCheckerPImpl::check(
           //   %1 = load_borrow %0
           //   %2 = copy_value %1
           //   %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2
-          if (auto *lbi = dyn_cast<LoadBorrowInst>(i->getOperand(0))) {
+          if (auto *lbi = dyn_cast<LoadBorrowInst>(orig)) {
             for (auto *use : markedInst->getConsumingUses()) {
               destroys.push_back(cast<DestroyValueInst>(use->getUser()));
             }
             while (!destroys.empty())
               destroys.pop_back_val()->eraseFromParent();
-            markedInst->replaceAllUsesWith(lbi);
+            markedInst->replaceAllUsesWith(replacement);
             markedInst->eraseFromParent();
             cvi->eraseFromParent();
             continue;
@@ -527,15 +527,14 @@ void MoveOnlyObjectCheckerPImpl::check(
           //   (%yield, ..., %handle) = begin_apply
           //   %copy = copy_value %yield
           //   %mark = mark_unresolved_noncopyable_value [no_consume_or_assign] %copy
-          if (isa_and_nonnull<BeginApplyInst>(
-                  i->getOperand(0)->getDefiningInstruction())) {
-            if (i->getOperand(0)->getOwnershipKind() == OwnershipKind::Guaranteed) {
+          if (isa_and_nonnull<BeginApplyInst>(orig->getDefiningInstruction())) {
+            if (orig->getOwnershipKind() == OwnershipKind::Guaranteed) {
               for (auto *use : markedInst->getConsumingUses()) {
                 destroys.push_back(cast<DestroyValueInst>(use->getUser()));
               }
               while (!destroys.empty())
                 destroys.pop_back_val()->eraseFromParent();
-              markedInst->replaceAllUsesWith(i->getOperand(0));
+              markedInst->replaceAllUsesWith(replacement);
               markedInst->eraseFromParent();
               cvi->eraseFromParent();
               continue;

test/Constraints/keypath_dynamic_member_lookup.swift

@@ -48,7 +48,7 @@ _ = lens.topLeft.x
 // CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs15WritableKeyPathCyxqd__G_tcluig
 // CHECK-NEXT: apply %68<Rectangle, Point>({{.*}})
 // CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs15WritableKeyPathCyxqd__G_tcluig
-// CHECK-NEXT: apply %75<Point, Int>({{.*}})
+// CHECK-NEXT: apply {{%[0-9]+}}<Point, Int>({{.*}})
 _ = lens.topLeft.y
 
 lens.topLeft = Lens(Point(x: 1, y: 2)) // Ok

test/IRGen/opaque_result_type_linkage.swift

@@ -10,6 +10,10 @@ struct G<T>: P {
   lazy var lazyVar: A = a
 
   var a: some Any {
+    b()
+  }
+
+  consuming func b() -> some Any {
     "hello"
   }
 }

test/SILGen/addressor_materialization_dependency.swift

@@ -0,0 +1,74 @@
+// RUN: %target-swift-emit-silgen %s | %FileCheck %s
+
+struct Foo: ~Copyable {
+    var bar: Bar<Baz>
+}
+
+struct Bar<T: ~Copyable>: ~Copyable {
+    var elements: T
+
+    subscript(i: Int) -> T {
+        unsafeAddress {
+            fatalError()
+        }
+    }
+}
+
+struct Baz: ~Copyable {
+    var storage: Int
+
+    borrowing func test() {}
+}
+
+// CHECK-LABEL: sil {{.*}} @${{.*}}4test1x
+func test(x: borrowing Foo) {
+    // The materialization of x.bar into a temporary to invoke the subscript
+    // addressor needs to cover the entire formal evaluation scope of the 
+    // projection, since the pointer may depend on the materialized
+    // representation
+
+    // CHECK: [[BAR_MAT:%.*]] = alloc_stack $Bar<Baz>
+    // CHECK: [[BAR_MAT_BORROW:%.*]] = store_borrow {{%.*}} to [[BAR_MAT]]
+    // CHECK: [[TEST_FN:%.*]] = function_ref @${{.*}}3BazV4test
+    // CHECK: apply [[TEST_FN]]
+    // CHECK: end_borrow [[BAR_MAT_BORROW]]
+    // CHECK: dealloc_stack [[BAR_MAT]]
+    x.bar[0].test()
+}
+
+// Value borrows should also cover the scope of the projected value, at
+// least until it's copied out if copyable.
+
+struct CopyableLoadable {
+    var x: AnyObject
+
+    subscript(i: Int) -> CopyableResult {
+        unsafeAddress {
+            fatalError()
+        }
+    }
+}
+
+struct CopyableResult {
+    var x: AnyObject
+
+    func test() {}
+}
+
+func copyableLoadable() -> CopyableLoadable { fatalError() }
+
+// CHECK-LABEL: sil {{.*}} @${{.*}}5test2
+func test2() {
+
+    // CHECK: [[BASE:%.*]] = begin_borrow
+    // CHECK: [[ADDRESSOR:%.*]] = function_ref @${{.*}}16CopyableLoadableVy{{.*}}
+    // CHECK: [[PTR:%.*]] = apply [[ADDRESSOR]]({{.*}}, [[BASE]])
+    // CHECK: [[RAWPTR:%.*]] = struct_extract [[PTR]]
+    // CHECK: [[ADDR:%.*]] = pointer_to_address [[RAWPTR]]
+    // CHECK: [[DEP_ADDR:%.*]] = mark_dependence [unresolved] [[ADDR]] on [[BASE]]
+    // CHECK: [[DEP_ADDR_ACCESS:%.*]] = begin_access [read] [unsafe] [[DEP_ADDR]]
+    // CHECK: load [copy] [[DEP_ADDR_ACCESS]]
+    // CHECK: end_access [[DEP_ADDR_ACCESS]]
+    // CHECK: end_borrow [[BASE]]
+    copyableLoadable()[0].test()
+}

test/SILGen/class_bound_protocols.swift

@@ -181,21 +181,8 @@ func takesInheritsMutatingMethod(x: inout InheritsMutatingMethod,
   // CHECK-NEXT: [[X_PAYLOAD:%.*]] = open_existential_ref [[X_VALUE]] : $any InheritsMutatingMethod to $@opened("{{.*}}", any InheritsMutatingMethod) Self
   // CHECK-NEXT: [[TEMPORARY:%.*]] = alloc_stack $@opened("{{.*}}", any InheritsMutatingMethod) Self
   // CHECK-NEXT: store [[X_PAYLOAD]] to [init] [[TEMPORARY]] : $*@opened("{{.*}}", any InheritsMutatingMethod) Self
-  // CHECK-NEXT: [[X_PAYLOAD_RELOADED:%.*]] = load_borrow [[TEMPORARY]]
-  //
-  // ** *NOTE* This extra copy is here since RValue invariants enforce that all
-  // ** loadable objects are actually loaded. So we form the RValue and
-  // ** load... only to then need to store the value back in a stack location to
-  // ** pass to an in_guaranteed method. PredictableMemOpts is able to handle this
-  // ** type of temporary codegen successfully.
-  // CHECK-NEXT: [[TEMPORARY_2:%.*]] = alloc_stack $@opened("{{.*}}", any InheritsMutatingMethod) Self
-  // CHECK-NEXT: [[SB:%.*]] = store_borrow [[X_PAYLOAD_RELOADED:%.*]] to [[TEMPORARY_2]]
-  // 
   // CHECK-NEXT: [[METHOD:%.*]] = witness_method $@opened("{{.*}}", any InheritsMutatingMethod) Self, #HasMutatingMethod.mutatingCounter!getter : <Self where Self : HasMutatingMethod> (Self) -> () -> Value, [[X_PAYLOAD]] : $@opened("{{.*}}", any InheritsMutatingMethod) Self : $@convention(witness_method: HasMutatingMethod) <τ_0_0 where τ_0_0 : HasMutatingMethod> (@in_guaranteed τ_0_0) -> Value
-  // CHECK-NEXT: [[RESULT_VALUE:%.*]] = apply [[METHOD]]<@opened("{{.*}}", any InheritsMutatingMethod) Self>([[SB]]) : $@convention(witness_method: HasMutatingMethod) <τ_0_0 where τ_0_0 : HasMutatingMethod> (@in_guaranteed τ_0_0) -> Value
-  // CHECK-NEXT:  end_borrow [[SB]]
-  // CHECK-NEXT: dealloc_stack  [[TEMPORARY_2]]
-  // CHECK-NEXT: end_borrow
+  // CHECK-NEXT: [[RESULT_VALUE:%.*]] = apply [[METHOD]]<@opened("{{.*}}", any InheritsMutatingMethod) Self>([[TEMPORARY]]) : $@convention(witness_method: HasMutatingMethod) <τ_0_0 where τ_0_0 : HasMutatingMethod> (@in_guaranteed τ_0_0) -> Value
   // CHECK-NEXT: destroy_addr
   // CHECK-NEXT: end_access [[X_ADDR]] : $*any InheritsMutatingMethod
   // CHECK-NEXT: ignored_use [[RESULT_VALUE]]