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

The return pointer may point into the materialized base value, so if the base needs
materialization, ensure that materialization covers any futher projection of the
value.

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]]