SILGen: Avoid redundant materialization when reabstracting a property base from loadable to in-memory.

The code here was materializing the value in this case, but then re-loading it, which is unnecessary
since call emission will materialize the value to match the callee's calling convention already.
It does look like a copy into formal evaluation scope is necessary to get the correct lifetimes
in some circumstances. The move-only checker doesn't like any additional copies at all because
it thinks they're consumes, so only borrow in the case where the value is move-only.

Author: jckarter

lib/SILGen/SILGenApply.cpp

@@ -6517,9 +6517,9 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() {
   if (selfParam.isConsumed() && !base.hasCleanup()) {
     base = base.copyUnmanaged(SGF, loc);
   }
-
-  // If the parameter is indirect, we need to drop the value into
-  // temporary memory.
+  // If the parameter is indirect, we'll need to drop the value into
+  // temporary memory. Make a copy scoped to the current formal access that
+  // we can materialize later.
   if (SGF.silConv.isSILIndirect(selfParam)) {
     // It's a really bad idea to materialize when we're about to
     // pass a value to an inout argument, because it's a really easy
@@ -6529,14 +6529,17 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() {
     // itself).
     assert(!selfParam.isIndirectMutating() &&
            "passing unmaterialized r-value as inout argument");
-
-    base = base.formallyMaterialize(SGF, loc);
-    auto shouldTake = IsTake_t(base.hasCleanup());
-    base = SGF.emitFormalAccessLoad(loc, base.forward(SGF),
-                                    SGF.getTypeLowering(baseLoweredType),
-                                    SGFContext(), shouldTake);
+           
+    // Avoid copying the base if it's move-only. It should be OK to do in this
+    // case since the move-only base will be accessed in a formal access scope
+    // as well. This isn't always true for copyable bases so be less aggressive
+    // in that case.
+    if (base.getType().isMoveOnly()) {
+      base = base.formalAccessBorrow(SGF, loc);
+    } else {
+      base = base.formalAccessCopy(SGF, loc);
+    }
   }
-
   return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base));
 }
 

test/SILGen/cf_members.swift

@@ -124,10 +124,8 @@ public func foo(_ x: Double) {
   // CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[Z]] : $*Struct1
   // CHECK: [[ZVAL:%.*]] = load [trivial] [[READ]]
   // CHECK: store [[ZVAL]] to [trivial] [[ZTMP:%.*]] :
-  // CHECK: [[ZVAL_2:%.*]] = load [trivial] [[ZTMP]]
-  // CHECK: store [[ZVAL_2]] to [trivial] [[ZTMP_2:%.*]] :
   // CHECK: [[GET:%.*]] = function_ref @IAMStruct1GetRadius : $@convention(c) (@in Struct1) -> Double
-  // CHECK: apply [[GET]]([[ZTMP_2]])
+  // CHECK: apply [[GET]]([[ZTMP]])
   _ = z.radius
   // CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[Z]] : $*Struct1
   // CHECK: [[ZVAL:%.*]] = load [trivial] [[READ]]

test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift

@@ -0,0 +1,53 @@
+// RUN: %target-swift-frontend -emit-sil -verify -enable-library-evolution -enable-experimental-feature MoveOnlyResilientTypes %s
+
+// Verify that call sequences that require reabstracting a noncopyable value
+// from a loadable representation to an in-memory one are properly allowed by
+// the move-only checker.
+
+public struct Foo: ~Copyable {
+    private var x: String = ""
+
+    public var isEmpty: Bool {
+        @_silgen_name("get_isEmpty") get
+    }
+}
+
+public struct Bar: ~Copyable {
+    public var x = Foo()
+
+    public consuming func foo() {
+        // `Foo` is internally a loadable type, but it's public with library
+        // evolution enabled, so public members like `isEmpty` take their
+        // argument indirectly.
+        if x.isEmpty {}
+        //else { bar(x) }
+    }
+}
+
+@_silgen_name("bar")
+func bar(_: consuming Foo)
+
+func foo(_ x: consuming Foo) {
+    // `[String]` is a loadable copyable type, but we're using the
+    // `consuming` modifier on `x` to suppress implicit copyability.
+    // `isEmpty` is a method from the Collection protocol, so it takes `self`
+    // generically and therefore indirectly.
+    if x.isEmpty {}
+    else { bar(x) }
+}
+
+func copyableBar(_: consuming [String]) {}
+
+func copyableFoo(prefix: consuming [String]) {
+    if  prefix.isEmpty {  }
+    else { copyableBar(prefix) }
+}
+
+struct CopyableFoo {
+    var prefix: [String]
+
+    consuming func foo() {
+        if  prefix.isEmpty {  }
+        else { copyableBar(prefix) }
+    }
+}