[region-isolation] Be explicit if an instruction will never actually assign.

Currently when we create an assign instruction, if we find that the result of
the instruction and the operand of the instruction reduce to the same element
representative, then we do not actually emit an assign.

For certain instructions this makes sense, but this is misleading for
instructions like copies (copy_value) and geps (struct_element_addr) that this
is always true for. Instead of attempting to assign and just have the builder
always clean this up... make it explicit with a new routine called
translateSILLookThrough. When this is called, we just look up the value and
assert.

Author: gottesmm

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -764,40 +764,61 @@ class PartitionOpTranslator {
         builder.addAssignFresh(value->getRepresentative());
   }
 
-  void translateSILAssign(SILValue tgt, SILValue src) {
-    return translateSILMultiAssign(TinyPtrVector<SILValue>(tgt),
+  /// Add a look through operation. This asserts that dest and src map to the
+  /// same ID. Should only be used on instructions that are always guaranteed to
+  /// have this property due to getUnderlyingTrackedValue looking through them.
+  ///
+  /// DISCUSSION: We use this to ensure that the connection in between
+  /// getUnderlyingTrackedValue and these instructions is enforced and
+  /// explicit. Previously, we always called translateSILAssign and relied on
+  /// the builder to recognize these cases and not create an assign
+  /// PartitionOp. Doing such a thing obscures what is actually happening.
+  void translateSILLookThrough(SILValue dest, SILValue src) {
+    auto srcID = tryToTrackValue(src);
+    auto destID = tryToTrackValue(dest);
+    assert(((!destID || !srcID) || destID->getID() == srcID->getID()) &&
+           "srcID and dstID are different?!");
+  }
+
+  void translateSILAssign(SILValue dest, SILValue src) {
+    return translateSILMultiAssign(TinyPtrVector<SILValue>(dest),
                                    TinyPtrVector<SILValue>(src));
   }
 
+  void translateSILAssign(SILInstruction *inst) {
+    return translateSILMultiAssign(inst->getResults(),
+                                   inst->getOperandValues());
+  }
+
   /// If the passed SILValue is NonSendable, then create a fresh region for it,
   /// otherwise do nothing.
   void translateSILAssignFresh(SILValue val) {
     return translateSILMultiAssign(TinyPtrVector<SILValue>(val),
                                    TinyPtrVector<SILValue>());
   }
 
-  void translateSILMerge(SILValue fst, SILValue snd) {
-    auto nonSendableFst = tryToTrackValue(fst);
-    auto nonSendableSnd = tryToTrackValue(snd);
-    if (!nonSendableFst || !nonSendableSnd)
+  void translateSILMerge(SILValue dest, SILValue src) {
+    auto trackableDest = tryToTrackValue(dest);
+    auto trackableSrc = tryToTrackValue(src);
+    if (!trackableDest || !trackableSrc)
       return;
-    builder.addMerge(nonSendableFst->getRepresentative(),
-                     nonSendableSnd->getRepresentative());
+    builder.addMerge(trackableDest->getRepresentative(),
+                     trackableSrc->getRepresentative());
   }
 
   /// If tgt is known to be unaliased (computed thropugh a combination of
   /// AccessStorage's inUniquelyIdenfitied check and a custom search for
   /// captures by applications), then these can be treated as assignments of tgt
   /// to src. If the tgt could be aliased, then we must instead treat them as
   /// merges, to ensure any aliases of tgt are also updated.
-  void translateSILStore(SILValue tgt, SILValue src) {
-    if (auto nonSendableTgt = tryToTrackValue(tgt)) {
+  void translateSILStore(SILValue dest, SILValue src) {
+    if (auto nonSendableTgt = tryToTrackValue(dest)) {
       // Stores to unaliased storage can be treated as assignments, not merges.
       if (nonSendableTgt.value().isNoAlias())
-        return translateSILAssign(tgt, src);
+        return translateSILAssign(dest, src);
 
       // Stores to possibly aliased storage must be treated as merges.
-      return translateSILMerge(tgt, src);
+      return translateSILMerge(dest, src);
     }
 
     // Stores to storage of non-Sendable type can be ignored.
@@ -866,7 +887,6 @@ class PartitionOpTranslator {
     case SILInstructionKind::DynamicFunctionRefInst:
     case SILInstructionKind::PreviousDynamicFunctionRefInst:
     case SILInstructionKind::GlobalAddrInst:
-    case SILInstructionKind::BaseAddrForOffsetInst:
     case SILInstructionKind::GlobalValueInst:
     case SILInstructionKind::IntegerLiteralInst:
     case SILInstructionKind::FloatLiteralInst:
@@ -880,64 +900,76 @@ class PartitionOpTranslator {
     case SILInstructionKind::SelectEnumInst:
       return translateSILSelectEnum(inst);
 
-    // The following instructions are treated as assignments of their result
-    // (tgt) to their operand (src). This could yield a variety of behaviors
-    // depending on the sendability of both src and tgt.
-    // TODO: we could reduce the size of PartitionOp sequences generated by
-    //       treating some of these as lookthroughs in getUnderlyingTrackedValue
-    //       instead of as assignments
-    case SILInstructionKind::AddressToPointerInst:
+    // These are instructions that we treat as true assigns since we want to
+    // error semantically upon them if there is a use of one of these. For
+    // example, a cast would be inappropriate here. This is implemented by
+    // propagating the operand's region into the result's region and by
+    // requiring all operands.
+    case SILInstructionKind::LoadInst:
+    case SILInstructionKind::LoadBorrowInst:
+    case SILInstructionKind::LoadWeakInst:
+    case SILInstructionKind::StrongCopyUnownedValueInst:
+    case SILInstructionKind::ClassMethodInst:
+    case SILInstructionKind::ObjCMethodInst:
+    case SILInstructionKind::SuperMethodInst:
+    case SILInstructionKind::ObjCSuperMethodInst:
+      return translateSILAssign(inst->getResult(0), inst->getOperand(0));
+
+    // Instructions that getUnderlyingTrackedValue is guaranteed to look through
+    // and whose operand and result are guaranteed to be mapped to the same
+    // underlying region.
+    //
+    // NOTE: translateSILLookThrough asserts that this property is true.
+    case SILInstructionKind::BeginAccessInst:
+    case SILInstructionKind::BeginBorrowInst:
     case SILInstructionKind::CopyValueInst:
+    case SILInstructionKind::ProjectBoxInst:
+    case SILInstructionKind::InitEnumDataAddrInst:
+    case SILInstructionKind::OpenExistentialAddrInst:
+    case SILInstructionKind::StructElementAddrInst:
+    case SILInstructionKind::TupleElementAddrInst:
+    case SILInstructionKind::UncheckedRefCastInst:
+    case SILInstructionKind::UncheckedTakeEnumDataAddrInst:
+    case SILInstructionKind::UpcastInst:
+      return translateSILLookThrough(inst->getResult(0), inst->getOperand(0));
+
+    // Just make the result part of the operand's region without requiring.
+    //
+    // This is appropriate for things like object casts and object
+    // geps.
+    case SILInstructionKind::AddressToPointerInst:
+    case SILInstructionKind::BaseAddrForOffsetInst:
     case SILInstructionKind::ConvertEscapeToNoEscapeInst:
     case SILInstructionKind::ConvertFunctionInst:
     case SILInstructionKind::CopyBlockInst:
     case SILInstructionKind::CopyBlockWithoutEscapingInst:
     case SILInstructionKind::IndexAddrInst:
     case SILInstructionKind::InitBlockStorageHeaderInst:
-    case SILInstructionKind::InitEnumDataAddrInst:
     case SILInstructionKind::InitExistentialAddrInst:
     case SILInstructionKind::InitExistentialRefInst:
-    case SILInstructionKind::LoadInst:
-    case SILInstructionKind::LoadBorrowInst:
-    case SILInstructionKind::LoadWeakInst:
-    case SILInstructionKind::OpenExistentialAddrInst:
     case SILInstructionKind::OpenExistentialBoxInst:
     case SILInstructionKind::OpenExistentialRefInst:
     case SILInstructionKind::PointerToAddressInst:
     case SILInstructionKind::ProjectBlockStorageInst:
     case SILInstructionKind::RefElementAddrInst:
-    case SILInstructionKind::ProjectBoxInst:
     case SILInstructionKind::RefToUnmanagedInst:
-    case SILInstructionKind::StrongCopyUnownedValueInst:
-    case SILInstructionKind::StructElementAddrInst:
     case SILInstructionKind::StructExtractInst:
     case SILInstructionKind::TailAddrInst:
     case SILInstructionKind::ThickToObjCMetatypeInst:
     case SILInstructionKind::ThinToThickFunctionInst:
-    case SILInstructionKind::TupleElementAddrInst:
     case SILInstructionKind::UncheckedAddrCastInst:
     case SILInstructionKind::UncheckedEnumDataInst:
     case SILInstructionKind::UncheckedOwnershipConversionInst:
-    case SILInstructionKind::UncheckedRefCastInst:
-    case SILInstructionKind::UncheckedTakeEnumDataAddrInst:
     case SILInstructionKind::UnmanagedToRefInst:
-    case SILInstructionKind::UpcastInst:
-
-    // Dynamic dispatch:
-    case SILInstructionKind::ClassMethodInst:
-    case SILInstructionKind::ObjCMethodInst:
-    case SILInstructionKind::SuperMethodInst:
-    case SILInstructionKind::ObjCSuperMethodInst:
-      return translateSILAssign(inst->getResult(0), inst->getOperand(0));
-    case SILInstructionKind::BeginBorrowInst: {
-      return translateSILAssign(inst->getResult(0), inst->getOperand(0));
-    }
+      return translateSILAssign(inst);
 
+    /// Enum inst is handled specially since if it does not have an argument,
+    /// we must assign fresh. Otherwise, we must propagate.
     case SILInstructionKind::EnumInst: {
       auto *ei = cast<EnumInst>(inst);
       if (ei->getNumOperands() == 0)
         return translateSILAssignFresh(ei);
-      return translateSILAssign(ei, ei->getOperand());
+      return translateSILAssign(ei);
     }
 
     // These are treated as stores - meaning that they could write values into
@@ -951,29 +983,29 @@ class PartitionOpTranslator {
     case SILInstructionKind::StoreWeakInst:
       return translateSILStore(inst->getOperand(1), inst->getOperand(0));
 
+    // Applies are handled specially since we need to merge their results.
     case SILInstructionKind::ApplyInst:
     case SILInstructionKind::BeginApplyInst:
     case SILInstructionKind::BuiltinInst:
     case SILInstructionKind::PartialApplyInst:
     case SILInstructionKind::TryApplyInst:
       return translateSILApply(inst);
 
-    case SILInstructionKind::DestructureTupleInst: {
-      // handle tuple destruction
-      auto *destructTupleInst = cast<DestructureTupleInst>(inst);
-      return translateSILMultiAssign(
-          destructTupleInst->getResults(),
-          TinyPtrVector<SILValue>(destructTupleInst->getOperand()));
-    }
-
-    // Handle instructions that aggregate their operands into a single structure
-    // - treated as a multi assign.
+    // These are used by SIL to disaggregate values together in a gep like
+    // way. We want to error on uses, not on the destructure itself, so we
+    // propagate.
+    case SILInstructionKind::DestructureTupleInst:
+    case SILInstructionKind::DestructureStructInst:
+      return translateSILMultiAssign(inst->getResults(),
+                                     inst->getOperandValues());
+
+    // These are used by SIL to aggregate values together in a gep like way. We
+    // want to look at uses of structs, not the struct uses itself. So just
+    // propagate.
     case SILInstructionKind::ObjectInst:
     case SILInstructionKind::StructInst:
     case SILInstructionKind::TupleInst:
-      return translateSILMultiAssign(
-          TinyPtrVector<SILValue>(inst->getResult(0)),
-          inst->getOperandValues());
+      return translateSILAssign(inst);
 
     // Handle returns and throws - require the operand to be non-transferred
     case SILInstructionKind::ReturnInst:
@@ -1071,25 +1103,6 @@ class PartitionOpTranslator {
     case SILInstructionKind::YieldInst: // TODO: yield should be handled
       return;
 
-    // We ignore begin_access since we look through them. We look through them
-    // since we want to treat the use of the begin_access as the semantic giving
-    // instruction. Otherwise, if we have a store after a transfer we will emit
-    // an error on the begin_access rather than allowing for the store to
-    // overwrite the original value. This would then cause an error.
-    //
-    // Example:
-    //
-    //   %0 = alloc_stack
-    //   store %1 to %0
-    //   apply %transfer(%0)
-    //   %2 = begin_access [modify] [static] %0
-    //   store %2 to %0
-    //
-    // If we treated a begin_access as an assign, we would require %0 to not be
-    // transferred at %2 even though we are about to overwrite it.
-    case SILInstructionKind::BeginAccessInst:
-      return;
-
     default:
       break;
     }

test/Concurrency/sendnonsendable_region_based_sendability.swift

@@ -682,12 +682,12 @@ func one_consume_many_require_varag(a : A) async {
 
     //TODO: find a way to make the type used in the diagnostic more specific than the signature type
     await a.foo_vararg(ns0, ns1, ns2);
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (3 access sites displayed)}}
+    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
 
     foo_noniso_vararg(ns0, ns3, ns4); // expected-tns-note {{access here could race}}
-    foo_noniso_vararg(ns3, ns1, ns4); // expected-tns-note {{access here could race}}
-    foo_noniso_vararg(ns4, ns3, ns2); // expected-tns-note {{access here could race}}
+    foo_noniso_vararg(ns3, ns1, ns4); // xpected-tns-note {{access here could race}}
+    foo_noniso_vararg(ns4, ns3, ns2); // xpected-tns-note {{access here could race}}
 }
 
 func one_consume_one_require_vararg(a : A) async {
@@ -696,10 +696,10 @@ func one_consume_one_require_vararg(a : A) async {
     let ns2 = NonSendable();
 
     await a.foo_vararg(ns0, ns1, ns2);
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (3 access sites displayed)}}
+    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
 
-    foo_noniso_vararg(ns0, ns1, ns2); // expected-tns-note 3{{access here could race}}
+    foo_noniso_vararg(ns0, ns1, ns2); // expected-tns-note 1{{access here could race}}
 }
 
 func many_consume_one_require_vararg(a : A) async {
@@ -714,13 +714,13 @@ func many_consume_one_require_vararg(a : A) async {
     // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
     await a.foo_vararg(ns4, ns1, ns4)
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
+    // xpected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
     await a.foo_vararg(ns5, ns5, ns2)
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
+    // xpected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
 
-    foo_noniso_vararg(ns0, ns1, ns2); //expected-tns-note 3{{access here could race}}
+    foo_noniso_vararg(ns0, ns1, ns2); // expected-tns-note {{access here could race}}
 }
 
 func many_consume_many_require_vararg(a : A) async {
@@ -737,15 +737,15 @@ func many_consume_many_require_vararg(a : A) async {
     // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
     await a.foo_vararg(ns4, ns1, ns4)
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
+    // xpected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
     await a.foo_vararg(ns5, ns5, ns2)
-    // expected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
+    // xpected-tns-warning @-1 {{passing argument of non-sendable type 'Any...' from nonisolated context to actor-isolated context at this call site could yield a race with accesses later in this function (1 access site displayed)}}
     // expected-complete-warning @-2 {{passing argument of non-sendable type 'Any...' into actor-isolated context may introduce data races}}
 
     foo_noniso_vararg(ns0, ns6, ns7); // expected-tns-note {{access here could race}}
-    foo_noniso_vararg(ns6, ns1, ns7); // expected-tns-note {{access here could race}}
-    foo_noniso_vararg(ns7, ns6, ns2); // expected-tns-note {{access here could race}}
+    foo_noniso_vararg(ns6, ns1, ns7); // xpected-tns-note {{access here could race}}
+    foo_noniso_vararg(ns7, ns6, ns2); // xpected-tns-note {{access here could race}}
 }
 
 enum E { // expected-complete-note {{consider making enum 'E' conform to the 'Sendable' protocol}}