Merge pull request #74123 from gottesmm/pr-9e8378fdeee3204a34f48ea8d2ff8f0be40a4674

[region-isolation] Make store_borrow a store operation that does not require

Author: gottesmm

Diff for: include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -1025,14 +1025,6 @@ struct PartitionOpEvaluator {
              "Assign PartitionOp should be passed 2 arguments");
       assert(p.isTrackingElement(op.getOpArgs()[1]) &&
              "Assign PartitionOp's source argument should be already tracked");
-      // If we are using a region that was transferred as our assignment source
-      // value... emit an error.
-      if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) {
-        for (auto transferredOperand : transferredOperandSet->data()) {
-          handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1],
-                                            transferredOperand);
-        }
-      }
       p.assignElement(op.getOpArgs()[0], op.getOpArgs()[1]);
       return;
     case PartitionOpKind::AssignFresh:
@@ -1122,20 +1114,6 @@ struct PartitionOpEvaluator {
              p.isTrackingElement(op.getOpArgs()[1]) &&
              "Merge PartitionOp's arguments should already be tracked");
 
-      // if attempting to merge a transferred region, handle the failure
-      if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[0])) {
-        for (auto transferredOperand : transferredOperandSet->data()) {
-          handleLocalUseAfterTransferHelper(op, op.getOpArgs()[0],
-                                            transferredOperand);
-        }
-      }
-      if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) {
-        for (auto transferredOperand : transferredOperandSet->data()) {
-          handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1],
-                                            transferredOperand);
-        }
-      }
-
       p.merge(op.getOpArgs()[0], op.getOpArgs()[1]);
       return;
     case PartitionOpKind::Require:

Diff for: lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1584,7 +1584,8 @@ class PartitionOpTranslator {
   void
   translateSILMultiAssign(const TargetRange &resultValues,
                           const SourceRange &sourceValues,
-                          SILIsolationInfo resultIsolationInfoOverride = {}) {
+                          SILIsolationInfo resultIsolationInfoOverride = {},
+                          bool requireSrcValues = true) {
     SmallVector<SILValue, 8> assignOperands;
     SmallVector<SILValue, 8> assignResults;
 
@@ -1631,9 +1632,17 @@ class PartitionOpTranslator {
       }
     }
 
-    // Require all srcs.
-    for (auto src : assignOperands)
-      builder.addRequire(src);
+    // Require all srcs if we are supposed to. (By default we do).
+    //
+    // DISCUSSION: The reason that this may be useful is for special
+    // instructions like store_borrow. On the one hand, we want store_borrow to
+    // act like a store in the sense that we want to combine the regions of its
+    // src and dest... but at the same time, we do not want to treat the store
+    // itself as a use of its parent value. We want that to be any subsequent
+    // uses of the store_borrow.
+    if (requireSrcValues)
+      for (auto src : assignOperands)
+        builder.addRequire(src);
 
     // Merge all srcs.
     for (unsigned i = 1; i < assignOperands.size(); i++) {
@@ -2082,21 +2091,28 @@ class PartitionOpTranslator {
   }
 
   template <typename Collection>
-  void translateSILMerge(SILValue dest, Collection collection) {
+  void translateSILMerge(SILValue dest, Collection collection,
+                         bool requireOperands = true) {
     auto trackableDest = tryToTrackValue(dest);
     if (!trackableDest)
       return;
     for (SILValue elt : collection) {
       if (auto trackableSrc = tryToTrackValue(elt)) {
+        if (requireOperands) {
+          builder.addRequire(trackableSrc->getRepresentative().getValue());
+          builder.addRequire(trackableDest->getRepresentative().getValue());
+        }
         builder.addMerge(trackableDest->getRepresentative().getValue(),
                          trackableSrc->getRepresentative().getValue());
       }
     }
   }
 
   template <>
-  void translateSILMerge<SILValue>(SILValue dest, SILValue src) {
-    return translateSILMerge(dest, TinyPtrVector<SILValue>(src));
+  void translateSILMerge<SILValue>(SILValue dest, SILValue src,
+                                   bool requireOperands) {
+    return translateSILMerge(dest, TinyPtrVector<SILValue>(src),
+                             requireOperands);
   }
 
   void translateSILAssignmentToTransferringParameter(TrackableValue destRoot,
@@ -2572,7 +2588,6 @@ CONSTANT_TRANSLATION(InitExistentialValueInst, LookThrough)
 CONSTANT_TRANSLATION(CopyAddrInst, Store)
 CONSTANT_TRANSLATION(ExplicitCopyAddrInst, Store)
 CONSTANT_TRANSLATION(StoreInst, Store)
-CONSTANT_TRANSLATION(StoreBorrowInst, Store)
 CONSTANT_TRANSLATION(StoreWeakInst, Store)
 CONSTANT_TRANSLATION(MarkUnresolvedMoveAddrInst, Store)
 CONSTANT_TRANSLATION(UncheckedRefCastAddrInst, Store)
@@ -2804,6 +2819,44 @@ LOOKTHROUGH_IF_NONSENDABLE_RESULT_AND_OPERAND(UncheckedTakeEnumDataAddrInst)
 // Custom Handling
 //
 
+TranslationSemantics
+PartitionOpTranslator::visitStoreBorrowInst(StoreBorrowInst *sbi) {
+  // A store_borrow is an interesting instruction since we are essentially
+  // temporarily binding an object value to an address... so really any uses of
+  // the address, we want to consider to be uses of the parent object. So we
+  // basically put source/dest into the same region, but do not consider the
+  // store_borrow itself to be a require use. This prevents the store_borrow
+  // from causing incorrect diagnostics.
+  SILValue destValue = sbi->getDest();
+  SILValue srcValue = sbi->getSrc();
+
+  auto nonSendableDest = tryToTrackValue(destValue);
+  if (!nonSendableDest)
+    return TranslationSemantics::Ignored;
+
+  // In the following situations, we can perform an assign:
+  //
+  // 1. A store to unaliased storage.
+  // 2. A store that is to an entire value.
+  //
+  // DISCUSSION: If we have case 2, we need to merge the regions since we
+  // are not overwriting the entire region of the value. This does mean that
+  // we artificially include the previous region that was stored
+  // specifically in this projection... but that is better than
+  // miscompiling. For memory like this, we probably need to track it on a
+  // per field basis to allow for us to assign.
+  if (nonSendableDest.value().isNoAlias() &&
+      !isProjectedFromAggregate(destValue)) {
+    translateSILMultiAssign(sbi->getResults(), sbi->getOperandValues(),
+                            SILIsolationInfo(), false /*require src*/);
+  } else {
+    // Stores to possibly aliased storage must be treated as merges.
+    translateSILMerge(destValue, srcValue, false /*require src*/);
+  }
+
+  return TranslationSemantics::Special;
+}
+
 TranslationSemantics
 PartitionOpTranslator::visitAllocStackInst(AllocStackInst *asi) {
   // Before we do anything, see if asi is Sendable or if it is non-Sendable,

Diff for: test/Concurrency/transfernonsendable.swift

@@ -15,7 +15,7 @@
 ////////////////////////
 
 /// Classes are always non-sendable, so this is non-sendable
-class NonSendableKlass { // expected-complete-note 51{{}}
+class NonSendableKlass { // expected-complete-note 53{{}}
   // expected-typechecker-only-note @-1 3{{}}
   // expected-tns-note @-2 {{}}
   var field: NonSendableKlass? = nil
@@ -1736,3 +1736,13 @@ func sendableGlobalActorIsolated() {
   }
   print(x) // expected-tns-note {{access can happen concurrently}}
 }
+
+// We do not get an error here since we are transferring x both times to a main
+// actor isolated thing function. We used to emit an error when using region
+// isolation since we would trip on the store_borrow we used to materialize the
+// value.
+func testIndirectParameterSameIsolationNoError() async {
+  let x = NonSendableKlass()
+  await transferToMain(x) // expected-complete-warning {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
+  await transferToMain(x) // expected-complete-warning {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
+}

Diff for: test/Concurrency/transfernonsendable_asynclet.swift

@@ -9,7 +9,7 @@
 ////////////////////////
 
 /// Classes are always non-sendable, so this is non-sendable
-class NonSendableKlass { // expected-complete-note 96{{}}
+class NonSendableKlass { // expected-complete-note 99{{}}
   // expected-tns-note @-1 {{}}
   var field: NonSendableKlass? = nil
   var field2: NonSendableKlass? = nil
@@ -34,6 +34,13 @@ final actor FinalActor {
   func useKlass(_ x: NonSendableKlass) {}
 }
 
+actor CustomActorInstance {}
+
+@globalActor
+struct CustomActor {
+  static let shared = CustomActorInstance()
+}
+
 func useInOut<T>(_ x: inout T) {}
 @discardableResult
 func useValue<T>(_ x: T) -> T { x }
@@ -42,6 +49,7 @@ func useValueWrapInOptional<T>(_ x: T) -> T? { x }
 @MainActor func returnValueFromMain<T>() async -> T { fatalError() }
 @MainActor func transferToMain<T>(_ t: T) async {}
 @MainActor func transferToMainInt<T>(_ t: T) async -> Int { 5 }
+@CustomActor func transferToCustomInt<T>(_ t: T) async -> Int { 5 }
 @MainActor func transferToMainIntOpt<T>(_ t: T) async -> Int? { 5 }
 
 func transferToNonIsolated<T>(_ t: T) async {}
@@ -306,18 +314,33 @@ func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr3() async {
   let _ = await y
 }
 
-// Make sure that we emit an error for transferToMainInt in the async val
-// function itself.
+// Make sure that we do not emit an error for transferToMainInt in the async val
+// function itself since we are sending the value to the same main actor
+// isolated use and transferring it into one async let variable.
 func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr4() async {
   let x = NonSendableKlass()
 
-  async let y = useValue(transferToMainInt(x) + transferToMainInt(x)) // expected-tns-warning {{sending 'x' risks causing data races}}
-  // expected-tns-note @-1 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}}
-  // expected-tns-note @-2:67 {{access can happen concurrently}}
+  async let y = useValue(transferToMainInt(x) + transferToMainInt(x))
+  // expected-complete-warning @-1 {{capture of 'x' with non-sendable type 'NonSendableKlass' in 'async let' binding}}
+  // expected-complete-warning @-2 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
+  // expected-complete-warning @-3 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
 
-  // expected-complete-warning @-4 {{capture of 'x' with non-sendable type 'NonSendableKlass' in 'async let' binding}}
-  // expected-complete-warning @-5 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
+  let _ = await y
+}
+
+// Make sure that we do emit an error since we are sending the value to two
+// different isolation domains in the async let.
+func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr5() async {
+  let x = NonSendableKlass()
+
+  async let y = useValue(transferToMainInt(x) + transferToCustomInt(x))
+  // expected-tns-warning @-1 {{sending 'x' risks causing data races}}
+  // expected-tns-note @-2 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}}
+  // expected-tns-note @-3:49 {{access can happen concurrently}}
+
+  // expected-complete-warning @-5 {{capture of 'x' with non-sendable type 'NonSendableKlass' in 'async let' binding}}
   // expected-complete-warning @-6 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
+  // expected-complete-warning @-7 {{passing argument of non-sendable type 'NonSendableKlass' into global actor 'CustomActor'-isolated context may introduce data races}}
 
   let _ = await y
 }