Merge pull request #72851 from gottesmm/enable-region-isolation-by-default

[region-isolation] Enable by default when strict concurrency is enabled

Author: gottesmm

include/swift/AST/ActorIsolation.h

@@ -111,6 +111,9 @@ class ActorIsolation {
 
   static ActorIsolation forActorInstanceSelf(ValueDecl *decl);
 
+  /// Create an ActorIsolation appropriate for a type that is self.
+  static ActorIsolation forActorInstanceSelf(NominalTypeDecl *decl);
+
   static ActorIsolation forActorInstanceParameter(NominalTypeDecl *actor,
                                                   unsigned parameterIndex) {
     return ActorIsolation(ActorInstance, actor, parameterIndex + 1);

include/swift/Option/FrontendOptions.td

@@ -1343,4 +1343,9 @@ def disable_experimental_parser_round_trip : Flag<["-"],
   "disable-experimental-parser-round-trip">,
   HelpText<"Disable round trip through the new swift parser">;
 
+def disable_strict_concurrency_region_based_isolation : Flag<["-"],
+  "disable-region-based-isolation-with-strict-concurrency">,
+  HelpText<"Disable region based isolation when running with strict concurrency enabled. Only enabled with asserts">,
+  Flags<[HelpHidden]>;
+
 } // end let Flags = [FrontendOption, NoDriverOption, HelpHidden]

include/swift/SILOptimizer/Analysis/RegionAnalysis.h

@@ -162,7 +162,7 @@ class regionanalysisimpl::TrackableValueState {
   }
 
   ActorIsolation getActorIsolation() const {
-    return regionInfo.getActorIsolation().value();
+    return regionInfo.getActorIsolation();
   }
 
   void mergeIsolationRegionInfo(SILIsolationInfo newRegionInfo) {

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -110,20 +110,19 @@ class SILIsolationInfo {
   // 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 *,
+    ActorIsolation,
     // The task isolated parameter when we find a task isolated value.
     SILValue
   > data;
   // clang-format on
 
-  SILIsolationInfo(Kind kind, std::optional<ActorIsolation> actorIsolation)
-      : kind(kind), data(actorIsolation) {}
-  SILIsolationInfo(Kind kind, NominalTypeDecl *decl) : kind(kind), data(decl) {}
+  SILIsolationInfo(ActorIsolation actorIsolation)
+      : kind(Actor), data(actorIsolation) {}
 
   SILIsolationInfo(Kind kind, SILValue value) : kind(kind), data(value) {}
 
+  SILIsolationInfo(Kind kind) : kind(kind), data() {}
+
 public:
   SILIsolationInfo() : kind(Kind::Unknown), data() {}
 
@@ -146,18 +145,11 @@ class SILIsolationInfo {
 
   void printForDiagnostics(llvm::raw_ostream &os) const;
 
-  std::optional<ActorIsolation> getActorIsolation() const {
+  ActorIsolation getActorIsolation() const {
     assert(kind == Actor);
-    assert(std::holds_alternative<std::optional<ActorIsolation>>(data) &&
+    assert(std::holds_alternative<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);
+    return std::get<ActorIsolation>(data);
   }
 
   SILValue getTaskIsolatedValue() const {
@@ -167,45 +159,31 @@ class SILIsolationInfo {
     return std::get<SILValue>(data);
   }
 
-  bool hasActorIsolation() const {
-    return kind == Actor &&
-           std::holds_alternative<std::optional<ActorIsolation>>(data);
-  }
-
-  bool hasActorInstance() const {
-    return kind == Actor && std::holds_alternative<NominalTypeDecl *>(data);
-  }
+  bool hasActorIsolation() const { return kind == Actor; }
 
   bool hasTaskIsolatedValue() const {
     return kind == Task && std::holds_alternative<SILValue>(data);
   }
 
-  /// If we actually have an actor decl, return that. Otherwise, see if we have
-  /// an actor isolation if we can find one in there. Returns nullptr if we
-  /// fail.
-  NominalTypeDecl *tryInferActorDecl() const;
-
   [[nodiscard]] SILIsolationInfo merge(SILIsolationInfo other) const;
 
   SILIsolationInfo withActorIsolated(ActorIsolation isolation) {
     return SILIsolationInfo::getActorIsolated(isolation);
   }
 
-  static SILIsolationInfo getDisconnected() { return {Kind::Disconnected, {}}; }
+  static SILIsolationInfo getDisconnected() { return {Kind::Disconnected}; }
 
   static SILIsolationInfo getActorIsolated(ActorIsolation actorIsolation) {
-    return {Kind::Actor, actorIsolation};
+    return {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 SILIsolationInfo getActorIsolated(NominalTypeDecl *nomDecl) {
-    auto actorIsolation = swift::getActorIsolation(nomDecl);
-    if (actorIsolation.isActorIsolated())
-      return getActorIsolated(actorIsolation);
-    if (nomDecl->isActor())
-      return {Kind::Actor, nomDecl};
-    return SILIsolationInfo();
+  static SILIsolationInfo getActorIsolated(NominalTypeDecl *typeDecl) {
+    if (typeDecl->isActor())
+      return {ActorIsolation::forActorInstanceSelf(typeDecl)};
+    auto isolation = swift::getActorIsolation(typeDecl);
+    if (isolation.isGlobalActor())
+      return {isolation};
+    return {};
   }
 
   static SILIsolationInfo getGlobalActorIsolated(Type globalActorType) {

lib/AST/Decl.cpp

@@ -11370,6 +11370,10 @@ ActorIsolation::forActorInstanceSelf(ValueDecl *decl) {
   return ActorIsolation(ActorInstance, dc->getSelfNominalTypeDecl(), 0);
 }
 
+ActorIsolation ActorIsolation::forActorInstanceSelf(NominalTypeDecl *selfDecl) {
+  return ActorIsolation(ActorInstance, selfDecl, 0);
+}
+
 NominalTypeDecl *ActorIsolation::getActor() const {
   assert(getKind() == ActorInstance ||
          getKind() == GlobalActor);

lib/DriverTool/sil_opt_main.cpp

@@ -537,6 +537,11 @@ struct SILOptOptions {
   llvm::cl::list<std::string> ClangXCC = llvm::cl::list<std::string>(
       "Xcc",
       llvm::cl::desc("option to pass to clang"));
+
+  llvm::cl::opt<bool> DisableRegionBasedIsolationWithStrictConcurrency =
+      llvm::cl::opt<bool>(
+          "disable-region-based-isolation-with-strict-concurrency",
+          llvm::cl::init(false));
 };
 
 /// Regular expression corresponding to the value given in one of the
@@ -701,9 +706,14 @@ int sil_opt_main(ArrayRef<const char *> argv, void *MainAddr) {
 
   Invocation.getLangOptions().UnavailableDeclOptimizationMode =
       options.UnavailableDeclOptimization;
-  if (options.StrictConcurrencyLevel.hasArgStr())
+  if (options.StrictConcurrencyLevel.hasArgStr()) {
     Invocation.getLangOptions().StrictConcurrencyLevel =
         options.StrictConcurrencyLevel;
+    if (options.StrictConcurrencyLevel == StrictConcurrency::Complete &&
+        !options.DisableRegionBasedIsolationWithStrictConcurrency) {
+      Invocation.getLangOptions().enableFeature(Feature::RegionBasedIsolation);
+    }
+  }
 
   Invocation.getDiagnosticOptions().VerifyMode =
     options.VerifyMode ? DiagnosticOptions::Verify : DiagnosticOptions::NoVerify;

lib/Frontend/CompilerInvocation.cpp

@@ -1060,6 +1060,16 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   if (Opts.StrictConcurrencyLevel == StrictConcurrency::Complete) {
     Opts.enableFeature(Feature::IsolatedDefaultValues);
     Opts.enableFeature(Feature::GlobalConcurrency);
+
+    // If asserts are enabled, allow for region based isolation to be disabled
+    // with a flag. This is intended only to be used with tests.
+    bool enableRegionIsolation = true;
+#ifndef NDEBUG
+    enableRegionIsolation =
+        !Args.hasArg(OPT_disable_strict_concurrency_region_based_isolation);
+#endif
+    if (enableRegionIsolation)
+      Opts.enableFeature(Feature::RegionBasedIsolation);
   }
 
   Opts.WarnImplicitOverrides =

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -624,11 +624,7 @@ void SILIsolationInfo::printForDiagnostics(llvm::raw_ostream &os) const {
     os << "disconnected";
     return;
   case Actor:
-    if (hasActorIsolation() && getActorIsolation()) {
-      getActorIsolation()->printForDiagnostics(os);
-    } else {
-      os << "actor-isolated";
-    }
+    getActorIsolation().printForDiagnostics(os);
     return;
   case Task:
     os << "task-isolated";

lib/SILOptimizer/Utils/PartitionUtils.cpp

@@ -93,10 +93,7 @@ SILIsolationInfo SILIsolationInfo::get(SILFunctionArgument *arg) {
     if (auto functionIsolation = arg->getFunction()->getActorIsolation()) {
       if (functionIsolation.isActorIsolated()) {
         if (auto *nomDecl = self->getType().getNominalOrBoundGenericNominal()) {
-          if (auto isolationInfo =
-                  SILIsolationInfo::getActorIsolated(nomDecl)) {
-            return isolationInfo;
-          }
+          return SILIsolationInfo::getActorIsolated(nomDecl);
         }
       }
     }
@@ -135,23 +132,6 @@ void SILIsolationInfo::print(llvm::raw_ostream &os) const {
   }
 }
 
-NominalTypeDecl *SILIsolationInfo::tryInferActorDecl() const {
-  if (hasActorIsolation()) {
-    auto actorIsolation = getActorIsolation();
-    if (auto *actor = actorIsolation->getActorOrNullPtr()) {
-      return actor;
-    }
-    return nullptr;
-  }
-
-  if (hasActorInstance()) {
-    auto actorDecl = getActorInstance();
-    return actorDecl;
-  }
-
-  return nullptr;
-}
-
 SILIsolationInfo SILIsolationInfo::merge(SILIsolationInfo other) const {
   // If we are greater than the other kind, then we are further along the
   // lattice. We ignore the change.
@@ -179,21 +159,9 @@ bool SILIsolationInfo::operator==(const SILIsolationInfo &other) const {
     return getTaskIsolatedValue() == other.getTaskIsolatedValue();
   case Actor:
     // First try to use actor isolation if we have them.
-    if (hasActorIsolation() && other.hasActorIsolation()) {
-      auto lhsIsolation = getActorIsolation();
-      auto rhsIsolation = other.getActorIsolation();
-      if (lhsIsolation && rhsIsolation)
-        return *lhsIsolation == *rhsIsolation;
-    }
-
-    // Otherwise, try to use the inferred actor decl.
-    auto *lhsDecl = tryInferActorDecl();
-    auto *rhsDecl = other.tryInferActorDecl();
-    if (lhsDecl && rhsDecl)
-      return lhsDecl == rhsDecl;
-
-    // Otherwise, false, they are not equal.
-    return false;
+    auto lhsIsolation = getActorIsolation();
+    auto rhsIsolation = other.getActorIsolation();
+    return lhsIsolation == rhsIsolation;
   }
 }
 
@@ -207,22 +175,8 @@ void SILIsolationInfo::Profile(llvm::FoldingSetNodeID &id) const {
     id.AddPointer(getTaskIsolatedValue());
     return;
   case Actor:
-    // We profile in integer cases here so that we can always distinguish in
-    // between the various cases and the non-case. Just being paranoid.
-    if (hasActorIsolation()) {
-      if (auto isolation = getActorIsolation()) {
-        id.AddInteger(1);
-        return isolation->Profile(id);
-      }
-    }
-
-    if (hasActorInstance()) {
-      id.AddInteger(2);
-      return id.AddPointer(getActorInstance());
-    }
-
-    id.AddInteger(3);
-    break;
+    getActorIsolation().Profile(id);
+    return;
   }
 }
 

test/Concurrency/LLDBDebuggerFunctionActorExtension.swift

@@ -1,7 +1,7 @@
 // RUN: %target-swift-frontend  -disable-availability-checking -debugger-support %s -emit-sil -o /dev/null -verify
 // RUN: %target-swift-frontend  -disable-availability-checking -debugger-support %s -emit-sil -o /dev/null -verify -strict-concurrency=targeted
+// RUN: %target-swift-frontend  -disable-availability-checking -debugger-support %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -disable-region-based-isolation-with-strict-concurrency
 // RUN: %target-swift-frontend  -disable-availability-checking -debugger-support %s -emit-sil -o /dev/null -verify -strict-concurrency=complete
-// RUN: %target-swift-frontend  -disable-availability-checking -debugger-support %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -enable-upcoming-feature RegionBasedIsolation
 
 // REQUIRES: concurrency
 // REQUIRES: asserts