Merge pull request swiftlang#83959 from kavon/rdar159079818-subscript-read-nc

silgen: allow borrow of subscript for noncopyables

Author: kavon

lib/SILGen/FormalEvaluation.cpp

@@ -195,6 +195,20 @@ void FormalEvaluationScope::verify() const {
   }
 }
 
+void FormalEvaluationScope::deferPop() && {
+  ASSERT(previous && "no previous scope to defer the pop for!");
+  auto &context = SGF.FormalEvalContext;
+
+  // Remove ourselves as the innermost scope of the chain.
+  ASSERT(context.innermostScope == this &&
+         "popping formal-evaluation scopes out of order");
+  context.innermostScope = previous;
+
+  // Clear-out our saved depth to deactivate our pop-on-deinit.
+  savedDepth.reset();
+}
+
+
 //===----------------------------------------------------------------------===//
 //                         Formal Evaluation Context
 //===----------------------------------------------------------------------===//

lib/SILGen/FormalEvaluation.h

@@ -224,6 +224,14 @@ class FormalEvaluationScope {
     savedDepth.reset();
   }
 
+  /// Defers the emission of clean-ups in this scope until the
+  /// immediate outer evaluation scope is popped.
+  ///
+  /// This is useful for getting-around tightly-nested formal
+  /// access scopes, when a borrow of a value needs to be
+  /// returned and copying the value is not an option.
+  void deferPop() &&;
+
   FormalEvaluationScope(const FormalEvaluationScope &) = delete;
   FormalEvaluationScope &operator=(const FormalEvaluationScope &) = delete;
 

lib/SILGen/SILGenDecl.cpp

@@ -1770,6 +1770,7 @@ void SILGenFunction::emitStmtCondition(StmtCondition Cond, JumpDest FalseDest,
       auto *expr = elt.getBoolean();
       // Evaluate the condition as an i1 value (guaranteed by Sema).
       FullExpr Scope(Cleanups, CleanupLocation(expr));
+      FormalEvaluationScope EvalScope(*this);
       booleanTestValue = emitRValue(expr).forwardAsSingleValue(*this, expr);
       booleanTestValue = emitUnwrapIntegerResult(expr, booleanTestValue);
       booleanTestLoc = expr;

lib/SILGen/SILGenExpr.cpp

@@ -412,6 +412,7 @@ void SILGenFunction::emitExprInto(Expr *E, Initialization *I,
     return;
   }
 
+  FormalEvaluationScope writeback(*this);
   RValue result = emitRValue(E, SGFContext(I));
   if (result.isInContext())
     return;
@@ -2792,6 +2793,20 @@ RValue RValueEmitter::visitSubscriptExpr(SubscriptExpr *E, SGFContext C) {
   // Any writebacks for this access are tightly scoped.
   FormalEvaluationScope scope(SGF);
 
+  // For a noncopyable resulting expression, try to borrow instead.
+  if (E->getType()->isNoncopyable()) {
+    LValue lv = SGF.emitLValue(E, SGFAccessKind::BorrowedObjectRead);
+    ManagedValue mv = SGF.emitRawProjectedLValue(E, std::move(lv));
+
+    // If we got back a borrow (aka +0) because of a coroutine read
+    // accessor, defer the end_apply to the outer evaluation scope.
+    // Otherwise, we can end any other formal accesses now.
+    if (mv.isPlusZero())
+      std::move(scope).deferPop();
+
+    return RValue(SGF, E, mv);
+  }
+
   LValue lv = SGF.emitLValue(E, SGFAccessKind::OwnedObjectRead);
   // We can't load at +0 without further analysis, since the formal access into
   // the lvalue will end immediately.
@@ -6143,6 +6158,8 @@ RValue RValueEmitter::visitOptionalEvaluationExpr(OptionalEvaluationExpr *E,
   SmallVector<ManagedValue, 1> results;
   SGF.emitOptionalEvaluation(E, E->getType(), results, C,
     [&](SmallVectorImpl<ManagedValue> &results, SGFContext primaryC) {
+      // Generate each result in its own evaluation scope.
+      FormalEvaluationScope evalScope(SGF);
       ManagedValue result;
       if (!emitOptimizedOptionalEvaluation(SGF, E, result, primaryC)) {
         result = SGF.emitRValueAsSingleValue(E->getSubExpr(), primaryC);
@@ -6476,7 +6493,7 @@ RValue RValueEmitter::emitForceValue(ForceValueExpr *loc, Expr *E,
 void SILGenFunction::emitOpenExistentialExprImpl(
        OpenExistentialExpr *E,
        llvm::function_ref<void(Expr *)> emitSubExpr) {
-  assert(isInFormalEvaluationScope());
+  ASSERT(isInFormalEvaluationScope());
 
   // Emit the existential value.
   if (E->getExistentialValue()->getType()->is<LValueType>()) {
@@ -6511,7 +6528,14 @@ RValue RValueEmitter::visitOpenExistentialExpr(OpenExistentialExpr *E,
     return RValue(SGF, E, *result);
   }
 
-  FormalEvaluationScope writebackScope(SGF);
+  // Introduce a fresh, nested evaluation scope for Copyable types.
+  // This is a bit of a hack, as it should always be up to our caller
+  // to establish the right level of formal access scope, in case we
+  // formally borrow the opened existential instead of copying it.
+  std::optional<FormalEvaluationScope> scope;
+  if (!E->getType()->isNoncopyable())
+    scope.emplace(SGF);
+
   return SGF.emitOpenExistentialExpr<RValue>(E,
                                              [&](Expr *subExpr) -> RValue {
                                                return visit(subExpr, C);
@@ -7496,17 +7520,17 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
   // arguments.
 
   FullExpr scope(Cleanups, CleanupLocation(E));
+  FormalEvaluationScope evalScope(*this);
+
   if (E->getType()->hasLValueType()) {
     // Emit the l-value, but don't perform an access.
-    FormalEvaluationScope scope(*this);
     emitLValue(E, SGFAccessKind::IgnoredRead);
     return;
   }
 
   // If this is a load expression, we try hard not to actually do the load
   // (which could materialize a potentially expensive value with cleanups).
   if (auto *LE = dyn_cast<LoadExpr>(E)) {
-    FormalEvaluationScope scope(*this);
     LValue lv = emitLValue(LE->getSubExpr(), SGFAccessKind::IgnoredRead);
 
     // If loading from the lvalue is guaranteed to have no side effects, we
@@ -7541,7 +7565,6 @@ void SILGenFunction::emitIgnoredExpr(Expr *E) {
   // emit the precondition(s) without having to load the value.
   SmallVector<ForceValueExpr *, 4> forceValueExprs;
   if (auto *LE = findLoadThroughForceValueExprs(E, forceValueExprs)) {
-    FormalEvaluationScope scope(*this);
     LValue lv = emitLValue(LE->getSubExpr(), SGFAccessKind::IgnoredRead);
 
     ManagedValue value;

lib/SILGen/SILGenFunction.h

@@ -2207,6 +2207,12 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
                                   TSanKind tsanKind = TSanKind::None);
   ManagedValue emitConsumedLValue(SILLocation loc, LValue &&src,
                                   TSanKind tsanKind = TSanKind::None);
+  /// Simply projects the LValue as a ManagedValue, allowing the caller
+  /// to check invariants and make adjustments manually.
+  ///
+  /// When in doubt, reach for emit[Borrowed|Consumed|etc]LValue.
+  ManagedValue emitRawProjectedLValue(SILLocation loc, LValue &&src,
+                                  TSanKind tsanKind = TSanKind::None);
   LValue emitOpenExistentialLValue(SILLocation loc,
                                    LValue &&existentialLV,
                                    CanArchetypeType openedArchetype,

lib/SILGen/SILGenLValue.cpp

@@ -5587,11 +5587,8 @@ ManagedValue SILGenFunction::emitAddressOfLValue(SILLocation loc,
          src.getAccessKind() == SGFAccessKind::OwnedAddressConsume ||
          src.getAccessKind() == SGFAccessKind::ReadWrite);
 
-  ManagedValue addr;
-  PathComponent &&component =
-    drillToLastComponent(loc, std::move(src), addr, tsanKind);
+  ManagedValue addr = emitRawProjectedLValue(loc, std::move(src), tsanKind);
 
-  addr = drillIntoComponent(*this, loc, std::move(component), addr, tsanKind);
   assert(addr.getType().isAddress() &&
          "resolving lvalue did not give an address");
   return ManagedValue::forLValue(addr.getValue());
@@ -5605,12 +5602,7 @@ ManagedValue SILGenFunction::emitBorrowedLValue(SILLocation loc,
          src.getAccessKind() == SGFAccessKind::BorrowedObjectRead);
   bool isIgnored = src.getAccessKind() == SGFAccessKind::IgnoredRead;
 
-  ManagedValue base;
-  PathComponent &&component =
-    drillToLastComponent(loc, std::move(src), base, tsanKind);
-
-  auto value =
-    drillIntoComponent(*this, loc, std::move(component), base, tsanKind);
+  auto value = emitRawProjectedLValue(loc, std::move(src), tsanKind);
 
   // If project() returned an owned value, and the caller cares, borrow it.
   if (value.hasCleanup() && !isIgnored)
@@ -5621,15 +5613,20 @@ ManagedValue SILGenFunction::emitBorrowedLValue(SILLocation loc,
 ManagedValue SILGenFunction::emitConsumedLValue(SILLocation loc, LValue &&src,
                                                 TSanKind tsanKind) {
   assert(isConsumeAccess(src.getAccessKind()));
+  return emitRawProjectedLValue(loc, std::move(src), tsanKind);
+}
 
+ManagedValue SILGenFunction::emitRawProjectedLValue(SILLocation loc,
+                                                LValue &&src,
+                                                TSanKind tsanKind) {
   ManagedValue base;
   PathComponent &&component =
-      drillToLastComponent(loc, std::move(src), base, tsanKind);
+    drillToLastComponent(loc, std::move(src), base, tsanKind);
 
-  auto value =
-      drillIntoComponent(*this, loc, std::move(component), base, tsanKind);
+  ManagedValue result =
+    drillIntoComponent(*this, loc, std::move(component), base, tsanKind);
 
-  return value;
+  return result;
 }
 
 LValue

lib/SILGen/SILGenStmt.cpp

@@ -729,6 +729,7 @@ void SILGenFunction::emitReturnExpr(SILLocation branchLoc,
   } else {
     // SILValue return.
     FullExpr scope(Cleanups, CleanupLocation(ret));
+    FormalEvaluationScope writeback(*this);
 
     // Does the return context require reabstraction?
     RValue RV;
@@ -774,7 +775,11 @@ void StmtEmitter::visitThrowStmt(ThrowStmt *S) {
     return;
   }
 
-  ManagedValue exn = SGF.emitRValueAsSingleValue(S->getSubExpr());
+  ManagedValue exn;
+  {
+    FormalEvaluationScope scope(SGF);
+    exn = SGF.emitRValueAsSingleValue(S->getSubExpr());
+  }
   SGF.emitThrow(S, exn, /* emit a call to willThrow */ true);
 }
 

test/SILGen/moveonly.swift

@@ -982,9 +982,13 @@ public struct LoadableSubscriptGetOnlyTester : ~Copyable {
 // CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
 // CHECK: [[MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[ACCESS]]
 // CHECK: [[LOAD:%.*]] = load_borrow [unchecked] [[MARK]]
-// CHECK: apply {{%.*}}([[M2_PROJECT]], {{%.*}}, [[LOAD]])
+// CHECK: [[STACK_ALLOC:%.*]] = alloc_stack $AddressOnlyProtocol
+// CHECK: [[M_STACK_ALLOC:%.*]] = mark_unresolved_non_copyable_value [consumable_and_assignable] [[STACK_ALLOC]]
+// CHECK: apply {{%.*}}([[M_STACK_ALLOC]], {{%.*}}, [[LOAD]])
 // CHECK: end_borrow [[LOAD]]
 // CHECK: end_access [[ACCESS]]
+// CHECK: copy_addr [take] [[M_STACK_ALLOC]] to [init] [[M2_PROJECT]]
+// CHECK: dealloc_stack [[STACK_ALLOC]]
 // CHECK: [[M2_MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[M2_PROJECT]]
 // CHECK: end_borrow [[M2_BORROW]]
 // CHECK: destroy_value [[M2_BOX]]

test/SILGen/moveonly_coroutine_access.swift

@@ -0,0 +1,124 @@
+// RUN: %target-swift-emit-silgen -DTRIVIAL %s | %FileCheck --check-prefix TRIVIAL %s
+// RUN: %target-swift-emit-sil -DTRIVIAL -sil-verify-all %s > /dev/null
+
+// RUN: %target-swift-emit-silgen -DLOADABLE %s | %FileCheck --check-prefix LOADABLE %s
+// RUN: %target-swift-emit-sil -DLOADABLE -sil-verify-all %s > /dev/null
+
+// RUN: %target-swift-emit-silgen -DADDRESS_ONLY %s | %FileCheck --check-prefix ADDRESS_ONLY %s
+// RUN: %target-swift-emit-sil -DADDRESS_ONLY -sil-verify-all %s > /dev/null
+
+class X {
+  let snc = SNC()
+}
+
+struct NC: ~Copyable {
+#if TRIVIAL
+  typealias T = Int
+  var x: T = 0
+#elseif LOADABLE
+  typealias T = X
+  var x: T = X()
+#elseif ADDRESS_ONLY
+  typealias T = Any
+  var x: T = X()
+#else
+#error("pick a mode")
+#endif
+
+  var consumingGetter: T {
+    consuming get { fatalError() }
+  }
+  var readCoroutine: T {
+    _read { yield x }
+  }
+  consuming func consumingFunc() -> T { fatalError() }
+  borrowing func borrowingFunc() -> T { fatalError() }
+
+  deinit { print("destroy") }
+}
+
+struct SNC: ~Copyable {
+  private var _data: NC = NC()
+
+  subscript(index: Int) -> NC {
+    _read { yield _data }
+    _modify { yield &_data }
+  }
+}
+
+func use(_ x: NC.T) {}
+
+
+// TRIVIAL-LABEL: sil hidden [ossa] @${{.*}}test_subscript_read3sncyAA3SNCV_tF : $@convention(thin) (@guaranteed SNC) -> () {
+// TRIVIAL:         ([[YIELDED:%.*]], [[HANDLE:%.*]]) = begin_apply
+// TRIVIAL:          [[COPY:%.*]] = copy_value [[YIELDED]]
+// TRIVIAL:          [[MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[COPY]]
+// TRIVIAL:          [[BORROW_MARK:%.*]] = begin_borrow [[MARK]]
+// TRIVIAL:          [[X:%.*]] = struct_extract [[BORROW_MARK]], #NC.x
+// TRIVIAL:          end_borrow [[BORROW_MARK]]
+// TRIVIAL:          destroy_value [[MARK]]
+// TRIVIAL:          = end_apply [[HANDLE]] as $()
+// TRIVIAL:          = apply {{%.*}}([[X]])
+
+
+// LOADABLE-LABEL: sil hidden [ossa] @${{.*}}test_subscript_read3sncyAA3SNCV_tF : $@convention(thin) (@guaranteed SNC) -> () {
+// LOADABLE:         ([[YIELDED:%.*]], [[HANDLE:%.*]]) = begin_apply
+// LOADABLE:          [[COPY:%.*]] = copy_value [[YIELDED]]
+// LOADABLE:          [[MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[COPY]]
+// LOADABLE:          [[BORROW_MARK:%.*]] = begin_borrow [[MARK]]
+// LOADABLE:          [[X:%.*]] = struct_extract [[BORROW_MARK]], #NC.x
+
+// LOADABLE:          [[X_COPY:%.*]] = copy_value [[X]]
+
+// LOADABLE:          end_borrow [[BORROW_MARK]]
+// LOADABLE:          destroy_value [[MARK]]
+// LOADABLE:          = end_apply [[HANDLE]] as $()
+
+// LOADABLE:          = apply {{%.*}}([[X_COPY]])
+
+
+
+// ADDRESS_ONLY-LABEL: sil hidden [ossa] @${{.*}}test_subscript_read3sncyAA3SNCV_tF : $@convention(thin) (@in_guaranteed SNC) -> () {
+// ADDRESS_ONLY:         ([[YIELDED:%.*]], [[HANDLE:%.*]]) = begin_apply
+
+// ADDRESS_ONLY:          [[MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[YIELDED]]
+// ADDRESS_ONLY:          [[X_FIELD_ADDR:%.*]] = struct_element_addr [[MARK]], #NC.x
+// ADDRESS_ONLY:          [[X_LOCAL_ADDR:%.*]] = alloc_stack $Any
+// ADDRESS_ONLY:          copy_addr [[X_FIELD_ADDR]] to [init] [[X_LOCAL_ADDR]]
+// ADDRESS_ONLY:          = end_apply [[HANDLE]] as $()
+// ADDRESS_ONLY:          = apply {{%.*}}([[X_LOCAL_ADDR]])
+// ADDRESS_ONLY:          destroy_addr [[X_LOCAL_ADDR]]
+// ADDRESS_ONLY:          dealloc_stack [[X_LOCAL_ADDR]]
+
+func test_subscript_read(snc: borrowing SNC) {
+  use(snc[0].x)
+}
+
+
+struct SNC_OPT: ~Copyable {
+  private var _data: NC? = NC()
+
+  subscript(index: Int) -> NC? {
+    _read { yield _data }
+    _modify { yield &_data }
+  }
+}
+
+// LOADABLE-LABEL:  sil hidden [ossa] @${{.*}}test_subscript_write3sncyAA7SNC_OPTVz_tF
+
+// For the snc[0].take(), ensure the end_apply happens after the call to take.
+// LOADABLE:          begin_access [modify]
+// LOADABLE:          begin_apply
+// LOADABLE:          apply
+// LOADABLE:          end_apply
+// LOADABLE:          end_access
+
+// LOADABLE:          begin_access [modify]
+// LOADABLE:          begin_apply
+// LOADABLE:          assign
+// LOADABLE:          end_apply
+// LOADABLE:          end_access
+func test_subscript_write(snc: inout SNC_OPT) {
+  let x = snc[0].take()
+  snc[0] = x
+}