[region-isolation] Clean up the handling of ref_element_addr let for Sendable types.

Specifically, if we access a Sendable type from a ref_element_addr and the field
is mutable, we need to treat the use as a require since we could race on writing
to the field. If the field is a let though (and thus immutable), we can still
ignore it.

I also used this as an opportunity to add SIL tests for ref_element_addr.

Author: gottesmm

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -2627,12 +2627,20 @@ TranslationSemantics PartitionOpTranslator::visitUnconditionalCheckedCastInst(
 // ref_element_addr to be merged into.
 TranslationSemantics
 PartitionOpTranslator::visitRefElementAddrInst(RefElementAddrInst *reai) {
-  // If we are accessing a let of a Sendable type, do not treat the
-  // ref_element_addr as an assign.
-  if (reai->getField()->isLet() && !isNonSendableType(reai->getType())) {
-    LLVM_DEBUG(llvm::dbgs() << "    Found a let! Not tracking!\n");
-    return TranslationSemantics::Ignored;
+  // If our field is a NonSendableType...
+  if (!isNonSendableType(reai->getType())) {
+    // And the field is a let... then ignore it. We know that we cannot race on
+    // any writes to the field.
+    if (reai->getField()->isLet()) {
+      LLVM_DEBUG(llvm::dbgs() << "    Found a let! Not tracking!\n");
+      return TranslationSemantics::Ignored;
+    }
+
+    // Otherwise, we need to treat the access to the field as a require since we
+    // could have a race on assignment to the class.
+    return TranslationSemantics::Require;
   }
+
   return TranslationSemantics::Assign;
 }
 

test/Concurrency/transfernonsendable_instruction_matching.sil

@@ -19,38 +19,49 @@ import Builtin
 
 class NonSendableKlass {}
 
+sil @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
+sil @useNonSendableKlass : $@convention(thin) (@guaranteed NonSendableKlass) -> ()
+sil @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
+
 final class SendableKlass : Sendable {}
 
+sil @transferSendableKlass : $@convention(thin) @async (@guaranteed SendableKlass) -> ()
+sil @constructSendableKlass : $@convention(thin) () -> @owned SendableKlass
+
+final class KlassContainingKlasses {
+  let nsImmutable : NonSendableKlass
+  var nsMutable : NonSendableKlass
+  let sImmutable : SendableKlass
+  var sMutable : SendableKlass
+}
+
+sil @transferKlassContainingKlasses : $@convention(thin) @async (@guaranteed KlassContainingKlasses) -> ()
+sil @useKlassContainingKlasses : $@convention(thin) (@guaranteed KlassContainingKlasses) -> ()
+sil @constructKlassContainingKlasses : $@convention(thin) () -> @owned KlassContainingKlasses
+
 @_moveOnly
 struct NonSendableMoveOnlyStruct {
   var ns: NonSendableKlass
 
   deinit
 }
 
+sil @constructMoveOnlyStruct : $@convention(thin) () -> @owned NonSendableMoveOnlyStruct
+sil @transferMoveOnlyStruct : $@convention(thin) @async (@guaranteed NonSendableMoveOnlyStruct) -> ()
+
 struct NonSendableStruct {
   var ns: NonSendableKlass
 }
 
+sil @constructStruct : $@convention(thin) () -> @owned NonSendableStruct
+sil @transferStruct : $@convention(thin) @async (@guaranteed NonSendableStruct) -> ()
+
 sil @transferRawPointer : $@convention(thin) @async (Builtin.RawPointer) -> ()
 sil @useRawPointer : $@convention(thin) (Builtin.RawPointer) -> ()
 
-sil @transferSendableKlass : $@convention(thin) @async (@guaranteed SendableKlass) -> ()
-sil @constructSendableKlass : $@convention(thin) () -> @owned SendableKlass
-
-sil @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
-sil @useNonSendableKlass : $@convention(thin) (@guaranteed NonSendableKlass) -> ()
-sil @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
-
 sil @transferIndirect : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> ()
 sil @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
 
-sil @constructMoveOnlyStruct : $@convention(thin) () -> @owned NonSendableMoveOnlyStruct
-sil @transferMoveOnlyStruct : $@convention(thin) @async (@guaranteed NonSendableMoveOnlyStruct) -> ()
-
-sil @constructStruct : $@convention(thin) () -> @owned NonSendableStruct
-sil @transferStruct : $@convention(thin) @async (@guaranteed NonSendableStruct) -> ()
-
 enum FakeOptional<T> {
 case none
 case some(T)
@@ -1091,4 +1102,82 @@ bb0:
   destroy_value %1 : $NonSendableKlass
   %9999 = tuple ()
   return %9999 : $()
+}
+
+sil [ossa] @ref_element_addr_test_nonsendable_mutable : $@async @convention(thin) () -> () {
+bb0:
+  %0 = function_ref @constructKlassContainingKlasses : $@convention(thin) () -> @owned KlassContainingKlasses
+  %1 = apply %0() : $@convention(thin) () -> @owned KlassContainingKlasses
+
+  %2 = begin_borrow %1 : $KlassContainingKlasses
+  %3 = ref_element_addr %2 : $KlassContainingKlasses, #KlassContainingKlasses.nsMutable
+
+  %f2 = function_ref @transferIndirect : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f2<NonSendableKlass>(%3) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
+
+  %f3 = function_ref @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  apply %f3<NonSendableKlass>(%3) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{access here could race}}
+
+  end_borrow %2 : $KlassContainingKlasses
+
+  destroy_value %1 : $KlassContainingKlasses
+  %9999 = tuple ()
+  return %9999 : $()
+}
+
+sil [ossa] @ref_element_addr_test_nonsendable_immutable : $@async @convention(thin) () -> () {
+bb0:
+  %0 = function_ref @constructKlassContainingKlasses : $@convention(thin) () -> @owned KlassContainingKlasses
+  %1 = apply %0() : $@convention(thin) () -> @owned KlassContainingKlasses
+
+  %2 = begin_borrow %1 : $KlassContainingKlasses
+  %3 = ref_element_addr %2 : $KlassContainingKlasses, #KlassContainingKlasses.nsImmutable
+
+  %f2 = function_ref @transferIndirect : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f2<NonSendableKlass>(%3) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
+
+  %f3 = function_ref @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
+  apply %f3<NonSendableKlass>(%3) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{access here could race}}
+
+  end_borrow %2 : $KlassContainingKlasses
+
+  destroy_value %1 : $KlassContainingKlasses
+  %9999 = tuple ()
+  return %9999 : $()
+}
+
+sil [ossa] @ref_element_addr_test_sendable_mutable : $@async @convention(thin) () -> () {
+bb0:
+  %0 = function_ref @constructKlassContainingKlasses : $@convention(thin) () -> @owned KlassContainingKlasses
+  %1 = apply %0() : $@convention(thin) () -> @owned KlassContainingKlasses
+
+  %f = function_ref @transferKlassContainingKlasses : $@convention(thin) @async (@guaranteed KlassContainingKlasses) -> ()
+  // TODO: Improve diagnostic to make it clear that the reason we are failing is
+  // that we could race on a write to the ref_element_addr.
+  apply [caller_isolation=nonisolated] [callee_isolation=global_actor]  %f(%1) : $@convention(thin) @async (@guaranteed KlassContainingKlasses) -> () // expected-warning {{transferring value of non-Sendable type 'KlassContainingKlasses' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
+
+  %2 = begin_borrow %1 : $KlassContainingKlasses
+  %3 = ref_element_addr %2 : $KlassContainingKlasses, #KlassContainingKlasses.sMutable // expected-note {{access here could race}}
+  end_borrow %2 : $KlassContainingKlasses
+
+  destroy_value %1 : $KlassContainingKlasses
+  %9999 = tuple ()
+  return %9999 : $()
+}
+
+sil [ossa] @ref_element_addr_test_sendable_immutable : $@async @convention(thin) () -> () {
+bb0:
+  %0 = function_ref @constructKlassContainingKlasses : $@convention(thin) () -> @owned KlassContainingKlasses
+  %1 = apply %0() : $@convention(thin) () -> @owned KlassContainingKlasses
+
+  %f = function_ref @transferKlassContainingKlasses : $@convention(thin) @async (@guaranteed KlassContainingKlasses) -> ()
+  apply [caller_isolation=nonisolated] [callee_isolation=global_actor]  %f(%1) : $@convention(thin) @async (@guaranteed KlassContainingKlasses) -> ()
+
+  %2 = begin_borrow %1 : $KlassContainingKlasses
+  %3 = ref_element_addr %2 : $KlassContainingKlasses, #KlassContainingKlasses.sImmutable
+  end_borrow %2 : $KlassContainingKlasses
+
+  destroy_value %1 : $KlassContainingKlasses
+  %9999 = tuple ()
+  return %9999 : $()
 }