Merge pull request #72222 from gottesmm/propagate-actor-information

[region-isolation] Propagate region information from the analysis into the diagnostic emitter and fix a bunch of issues on the way

Author: gottesmm

include/swift/AST/ActorIsolation.h

@@ -274,6 +274,9 @@ class ActorIsolation {
     llvm_unreachable("Covered switch isn't covered?!");
   }
 
+  void printForDiagnostics(llvm::raw_ostream &os,
+                           StringRef openingQuotationMark = "'") const;
+
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
 };
 

include/swift/AST/DiagnosticsSIL.def

@@ -954,10 +954,10 @@ ERROR(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, non
       "binding of non-Sendable type %0 accessed after being transferred; later accesses could race",
       (Type))
 ERROR(regionbasedisolation_arg_transferred, none,
-      "task isolated value of type %0 transferred to %1 context; later accesses to value could race",
-      (Type, ActorIsolation))
+      "%0 value of type %1 transferred to %2 context; later accesses to value could race",
+      (StringRef, Type, ActorIsolation))
 ERROR(regionbasedisolation_arg_passed_to_strongly_transferred_param, none,
-      "task isolated value of type %0 passed as a strongly transferred parameter; later accesses could race",
+      "task-isolated value of type %0 passed as a strongly transferred parameter; later accesses could race",
       (Type))
 NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
      "value is %0 since it is in the same region as %1",
@@ -974,7 +974,7 @@ ERROR(regionbasedisolation_stronglytransfer_assignment_yields_race_name, none,
       "assigning %0 to transferring parameter %1 may cause a race",
       (Identifier, Identifier))
 NOTE(regionbasedisolation_stronglytransfer_taskisolated_assign_note, none,
-      "%0 is a task isolated value that is assigned into transferring parameter %1. Transferred uses of %1 may race with caller uses of %0",
+      "%0 is a task-isolated value that is assigned into transferring parameter %1. Transferred uses of %1 may race with caller uses of %0",
       (Identifier, Identifier))
 
 NOTE(regionbasedisolation_named_info_transfer_yields_race, none,
@@ -987,7 +987,7 @@ NOTE(regionbasedisolation_named_info_isolated_capture, none,
      "%1 value %0 is captured by %2 closure. Later local uses could race",
      (Identifier, ActorIsolation, ActorIsolation))
 NOTE(regionbasedisolation_named_arg_info, none,
-     "Transferring task isolated function argument %0 could yield races with caller uses",
+     "Transferring task-isolated function argument %0 could yield races with caller uses",
      (Identifier))
 NOTE(regionbasedisolation_named_stronglytransferred_binding, none,
      "Cannot access a transferring parameter after the parameter has been transferred", ())

include/swift/SILOptimizer/Analysis/RegionAnalysis.h

@@ -19,6 +19,7 @@
 #include "swift/SILOptimizer/Utils/PartitionUtils.h"
 
 #include <optional>
+#include <variant>
 
 namespace swift {
 
@@ -125,24 +126,166 @@ enum class TrackableValueFlag {
 
   /// Set to true if this TrackableValue's representative is Sendable.
   isSendable = 0x2,
-
-  /// Set to true if this TrackableValue is a non-sendable object derived from
-  /// an actor. Example: a value loaded from a ref_element_addr from an actor.
-  ///
-  /// NOTE: We track values with an actor representative even though actors are
-  /// sendable to be able to properly identify values that escape an actor since
-  /// if we escape an actor into a closure, we want to mark the closure as actor
-  /// derived.
-  isActorDerived = 0x4,
 };
 
 using TrackedValueFlagSet = OptionSet<TrackableValueFlag>;
 
+class ValueIsolationRegionInfo {
+public:
+  /// The lattice is:
+  ///
+  /// Unknown -> Disconnected -> TransferringParameter -> Task -> Actor.
+  ///
+  /// Unknown means no information. We error when merging on it.
+  enum Kind {
+    Unknown,
+    Disconnected,
+    Task,
+    Actor,
+  };
+
+private:
+  Kind kind;
+  // clang-format off
+  std::variant<
+    // Used for actor isolated when we have ActorIsolation info from the AST.
+    std::optional<ActorIsolation>,
+    // Used for actor isolation when we infer the actor at the SIL level.
+    NominalTypeDecl *,
+    // The task isolated parameter when we find a task isolated value.
+    SILValue
+  > data;
+  // clang-format on
+
+  ValueIsolationRegionInfo(Kind kind,
+                           std::optional<ActorIsolation> actorIsolation)
+      : kind(kind), data(actorIsolation) {}
+  ValueIsolationRegionInfo(Kind kind, NominalTypeDecl *decl)
+      : kind(kind), data(decl) {}
+
+  ValueIsolationRegionInfo(Kind kind, SILValue value)
+      : kind(kind), data(value) {}
+
+public:
+  ValueIsolationRegionInfo() : kind(Kind::Unknown), data() {}
+
+  operator bool() const { return kind != Kind::Unknown; }
+
+  operator Kind() const { return kind; }
+
+  Kind getKind() const { return kind; }
+
+  bool isDisconnected() const { return kind == Kind::Disconnected; }
+  bool isActorIsolated() const { return kind == Kind::Actor; }
+  bool isTaskIsolated() const { return kind == Kind::Task; }
+
+  void print(llvm::raw_ostream &os) const {
+    switch (Kind(*this)) {
+    case Unknown:
+      os << "unknown";
+      return;
+    case Disconnected:
+      os << "disconnected";
+      return;
+    case Actor:
+      os << "actor";
+      return;
+    case Task:
+      os << "task";
+      return;
+    }
+  }
+
+  void printForDiagnostics(llvm::raw_ostream &os) const;
+
+  SWIFT_DEBUG_DUMP {
+    print(llvm::dbgs());
+    llvm::dbgs() << '\n';
+  }
+
+  std::optional<ActorIsolation> getActorIsolation() const {
+    assert(kind == Actor);
+    assert(std::holds_alternative<std::optional<ActorIsolation>>(data) &&
+           "Doesn't have an actor isolation?!");
+    return std::get<std::optional<ActorIsolation>>(data);
+  }
+
+  NominalTypeDecl *getActorInstance() const {
+    assert(kind == Actor);
+    assert(std::holds_alternative<NominalTypeDecl *>(data) &&
+           "Doesn't have an actor instance?!");
+    return std::get<NominalTypeDecl *>(data);
+  }
+
+  SILValue getTaskIsolatedValue() const {
+    assert(kind == Task);
+    assert(std::holds_alternative<SILValue>(data) &&
+           "Doesn't have a task isolated value");
+    return std::get<SILValue>(data);
+  }
+
+  bool hasActorIsolation() const {
+    return std::holds_alternative<std::optional<ActorIsolation>>(data);
+  }
+
+  bool hasActorInstance() const {
+    return std::holds_alternative<NominalTypeDecl *>(data);
+  }
+
+  bool hasTaskIsolatedValue() const {
+    return std::holds_alternative<SILValue>(data);
+  }
+
+  [[nodiscard]] ValueIsolationRegionInfo
+  merge(ValueIsolationRegionInfo other) const {
+    // If we are greater than the other kind, then we are further along the
+    // lattice. We ignore the change.
+    if (unsigned(other.kind) < unsigned(kind))
+      return *this;
+
+    assert(kind != ValueIsolationRegionInfo::Actor &&
+           "Actor should never be merged with another actor?!");
+
+    // Otherwise, take the other value.
+    return other;
+  }
+
+  ValueIsolationRegionInfo withActorIsolated(ActorIsolation isolation) {
+    return ValueIsolationRegionInfo::getActorIsolated(isolation);
+  }
+
+  static ValueIsolationRegionInfo getDisconnected() {
+    return {Kind::Disconnected, {}};
+  }
+
+  static ValueIsolationRegionInfo
+  getActorIsolated(ActorIsolation actorIsolation) {
+    return {Kind::Actor, actorIsolation};
+  }
+
+  /// Sometimes we may have something that is actor isolated or that comes from
+  /// a type. First try getActorIsolation and otherwise, just use the type.
+  static ValueIsolationRegionInfo getActorIsolated(NominalTypeDecl *nomDecl) {
+    auto actorIsolation = swift::getActorIsolation(nomDecl);
+    if (actorIsolation.isActorIsolated())
+      return getActorIsolated(actorIsolation);
+    if (nomDecl->isActor())
+      return {Kind::Actor, nomDecl};
+    return {};
+  }
+
+  static ValueIsolationRegionInfo getTaskIsolated(SILValue value) {
+    return {Kind::Task, value};
+  }
+};
+
 } // namespace regionanalysisimpl
 
 class regionanalysisimpl::TrackableValueState {
   unsigned id;
   TrackedValueFlagSet flagSet = {TrackableValueFlag::isMayAlias};
+  ValueIsolationRegionInfo regionInfo =
+      ValueIsolationRegionInfo::getDisconnected();
 
 public:
   TrackableValueState(unsigned newID) : id(newID) {}
@@ -159,10 +302,20 @@ class regionanalysisimpl::TrackableValueState {
 
   bool isNonSendable() const { return !isSendable(); }
 
-  bool isActorDerived() const {
-    return flagSet.contains(TrackableValueFlag::isActorDerived);
+  ValueIsolationRegionInfo::Kind getRegionInfoKind() {
+    return regionInfo.getKind();
+  }
+
+  ActorIsolation getActorIsolation() const {
+    return regionInfo.getActorIsolation().value();
+  }
+
+  void mergeIsolationRegionInfo(ValueIsolationRegionInfo newRegionInfo) {
+    regionInfo = regionInfo.merge(newRegionInfo);
   }
 
+  ValueIsolationRegionInfo getIsolationRegionInfo() const { return regionInfo; }
+
   TrackableValueID getID() const { return TrackableValueID(id); }
 
   void addFlag(TrackableValueFlag flag) { flagSet |= flag; }
@@ -173,7 +326,9 @@ class regionanalysisimpl::TrackableValueState {
     os << "TrackableValueState[id: " << id
        << "][is_no_alias: " << (isNoAlias() ? "yes" : "no")
        << "][is_sendable: " << (isSendable() ? "yes" : "no")
-       << "][is_actor_derived: " << (isActorDerived() ? "yes" : "no") << "].";
+       << "][region_value_kind: ";
+    getIsolationRegionInfo().print(os);
+    os << "].";
   }
 
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
@@ -215,6 +370,7 @@ class regionanalysisimpl::RepresentativeValue {
 
   SILValue getValue() const { return value.get<SILValue>(); }
   SILValue maybeGetValue() const { return value.dyn_cast<SILValue>(); }
+  bool hasRegionIntroducingInst() const { return value.is<SILInstruction *>(); }
   SILInstruction *getActorRegionIntroducingInst() const {
     return value.get<SILInstruction *>();
   }
@@ -257,7 +413,9 @@ class regionanalysisimpl::TrackableValue {
 
   bool isNonSendable() const { return !isSendable(); }
 
-  bool isActorDerived() const { return valueState.isActorDerived(); }
+  ValueIsolationRegionInfo getIsolationRegionInfo() const {
+    return valueState.getIsolationRegionInfo();
+  }
 
   TrackableValueID getID() const {
     return TrackableValueID(valueState.getID());
@@ -293,6 +451,7 @@ class RegionAnalysisValueMap {
   using TrackableValueState = regionanalysisimpl::TrackableValueState;
   using TrackableValueID = Element;
   using RepresentativeValue = regionanalysisimpl::RepresentativeValue;
+  using ValueIsolationRegionInfo = regionanalysisimpl::ValueIsolationRegionInfo;
 
 private:
   /// A map from the representative of an equivalence class of values to their
@@ -308,11 +467,6 @@ class RegionAnalysisValueMap {
       equivalenceClassValuesToState;
   llvm::DenseMap<unsigned, RepresentativeValue> stateIndexToEquivalenceClass;
 
-  /// A list of values that can never be transferred.
-  ///
-  /// This only includes function arguments.
-  std::vector<TrackableValueID> neverTransferredValueIDs;
-
   SILFunction *fn;
 
 public:
@@ -326,13 +480,12 @@ class RegionAnalysisValueMap {
   /// value" returns an empty SILValue.
   SILValue maybeGetRepresentative(Element trackableValueID) const;
 
-  bool isActorDerived(Element trackableValueID) const;
-
-  ArrayRef<Element> getNonTransferrableElements() const {
-    return neverTransferredValueIDs;
-  }
+  /// Returns the fake "representative value" for this element if it
+  /// exists. Returns nullptr otherwise.
+  SILInstruction *maybeGetActorIntroducingInst(Element trackableValueID) const;
 
-  void sortUniqueNeverTransferredValues();
+  ValueIsolationRegionInfo getIsolationRegion(Element trackableValueID) const;
+  ValueIsolationRegionInfo getIsolationRegion(SILValue trackableValueID) const;
 
   void print(llvm::raw_ostream &os) const;
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
@@ -341,15 +494,23 @@ class RegionAnalysisValueMap {
   getTrackableValue(SILValue value,
                     bool isAddressCapturedByPartialApply = false) const;
 
+  /// An actor introducing inst is an instruction that doesn't have any
+  /// non-Sendable parameters and produces a new value that has to be actor
+  /// isolated.
+  ///
+  /// This is just for looking up the ValueIsolationRegionInfo for a
+  /// instructionInst if we have one. So it is a find like function.
+  std::optional<TrackableValue> getTrackableValueForActorIntroducingInst(
+      SILInstruction *introducingInst) const;
+
 private:
   std::optional<TrackableValue> getValueForId(TrackableValueID id) const;
   std::optional<TrackableValue> tryToTrackValue(SILValue value) const;
   TrackableValue
-  getActorIntroducingRepresentative(SILInstruction *introducingInst) const;
-  bool markValueAsActorDerived(SILValue value);
-  void addNeverTransferredValueID(TrackableValueID valueID) {
-    neverTransferredValueIDs.push_back(valueID);
-  }
+  getActorIntroducingRepresentative(SILInstruction *introducingInst,
+                                    ValueIsolationRegionInfo isolation) const;
+  bool mergeIsolationRegionInfo(SILValue value,
+                                ValueIsolationRegionInfo isolation);
   bool valueHasID(SILValue value, bool dumpIfHasNoID = false);
   TrackableValueID lookupValueID(SILValue value);
 };