Merge pull request #21779 from atrick/fix-merge-access

Reduce compile time for large strongly connected call graphs.

Author: atrick

include/swift/SILOptimizer/Analysis/AccessedStorageAnalysis.h

@@ -15,6 +15,10 @@
 // used by AccessEnforcementOpts to locally fold access scopes and remove
 // dynamic checks based on whole module analysis.
 //
+// This analysis may return conservative results by setting
+// FunctionAccessedStorage.unidentifiedAccess. This does not imply that all
+// accesses within the function have Unidentified AccessedStorage.
+//
 // Note: This interprocedural analysis can be easily augmented to simultaneously
 // compute FunctionSideEffects, without using a separate analysis, by adding
 // FunctionSideEffects as a member of FunctionAccessedStorage. However, passes
@@ -125,7 +129,10 @@ namespace swift {
 /// additional StorageAccessInfo bits are recorded as results of this analysis.
 ///
 /// Any unidentified accesses are summarized as a single unidentifiedAccess
-/// property.
+/// property. This property may also be used to conservatively summarize
+/// results, either because the call graph is unknown or the access sets are too
+/// large. It does not imply that all accesses have Unidentified
+/// AccessedStorage, which is never allowed for class or global access.
 class FunctionAccessedStorage {
   using AccessedStorageSet = llvm::SmallDenseSet<StorageAccessInfo, 8>;
 
@@ -164,6 +171,9 @@ class FunctionAccessedStorage {
     unidentifiedAccess = None;
   }
 
+  /// Return true if these effects are fully conservative.
+  bool hasWorstEffects() { return unidentifiedAccess == SILAccessKind::Modify; }
+
   /// Sets the most conservative effects, if we don't know anything about the
   /// function.
   void setWorstEffects() {

lib/SILOptimizer/Analysis/AccessedStorageAnalysis.cpp

@@ -151,29 +151,43 @@ bool FunctionAccessedStorage::updateUnidentifiedAccess(
 // substitution will be performed if possible. However, there's no guarantee
 // that the merged access values will belong to this function.
 //
-// Note that we may have `this` == `other` for self-recursion. We still need to
-// propagate and merge in that case in case arguments are recursively dependent.
+// Return true if these results changed, requiring further propagation through
+// the call graph.
 bool FunctionAccessedStorage::mergeAccesses(
     const FunctionAccessedStorage &other,
     std::function<StorageAccessInfo(const StorageAccessInfo &)>
       transformStorage) {
 
-  // Insertion in DenseMap invalidates the iterator in the rare case of
-  // self-recursion (`this` == `other`) that passes accessed storage though an
-  // argument. Rather than complicate the code, make a temporary copy of the
-  // AccessedStorage.
-  //
-  // Also note that the storageAccessIndex from otherStorage is relative to its
-  // original context and should not be copied into this context.
-  SmallVector<StorageAccessInfo, 8> otherStorageAccesses;
-  otherStorageAccesses.reserve(other.storageAccessSet.size());
-  otherStorageAccesses.append(other.storageAccessSet.begin(),
-                              other.storageAccessSet.end());
+  // The cost of BottomUpIPAnalysis can be quadratic for large recursive call
+  // graphs. That cost is multiplied by the size of storageAccessSet. Slowdowns
+  // can occur ~1000 elements. 200 is large enough to cover "normal" code,
+  // while ensuring compile time isn't affected.
+  if (storageAccessSet.size() > 200) {
+    llvm::dbgs() << "BIG SET " << storageAccessSet.size() << "\n";
+    setWorstEffects();
+    return true;
+  }
+  // To save compile time, if this storage already has worst-case effects, avoid
+  // growing its storageAccessSet.
+  if (hasWorstEffects())
+    return false;
 
+  // When `this` == `other` (for self-recursion), insertion in DenseMap
+  // invalidates the iterator. We still need to propagate and merge in that case
+  // because arguments can be recursively dependent. The alternative would be
+  // treating all self-recursion conservatively.
+  const FunctionAccessedStorage *otherFunctionAccesses = &other;
+  FunctionAccessedStorage functionAccessCopy;
+  if (this == &other) {
+    functionAccessCopy = other;
+    otherFunctionAccesses = &functionAccessCopy;
+  }
   bool changed = false;
-  for (auto &rawStorageInfo : otherStorageAccesses) {
+  // Nondeterminstically iterate for the sole purpose of inserting into another
+  // unordered set.
+  for (auto &rawStorageInfo : otherFunctionAccesses->storageAccessSet) {
     const StorageAccessInfo &otherStorageInfo =
-      transformStorage(rawStorageInfo);
+        transformStorage(rawStorageInfo);
     // If transformStorage() returns invalid storage object for local storage,
     // that should not be merged with the caller.
     if (!otherStorageInfo)

lib/SILOptimizer/Transforms/AccessEnforcementWMO.cpp

@@ -15,11 +15,11 @@
 ///
 /// This maps each access of identified storage onto a disjoint access
 /// location. Local accesses (Box and Stack) already have unique AccessedStorage
-/// and should be removed by an earlier function pass. This pass handles Class
-/// and Global access by partitioning their non-unique AccessedStorage objects
-/// into unique DisjointAccessLocations. These disjoint access locations may be
-/// accessed across multiple functions, so a module pass is required to identify
-/// and optimize them.
+/// and should be removed by an earlier function pass if possible. This pass
+/// handles Class and Global access by partitioning their non-unique
+/// AccessedStorage objects into unique DisjointAccessLocations. These disjoint
+/// access locations may be accessed across multiple functions, so a module pass
+/// is required to identify and optimize them.
 ///
 /// Class accesses are partitioned by their fully qualified property
 /// name. Global accesses are partitioned by the global variable name. Argument
@@ -48,7 +48,9 @@
 ///
 /// Note: This optimization must be aware of all possible access to a Class or
 /// Global address. This includes unpaired access instructions and keypath
-/// instructions. Ignoring any access pattern would weaken enforcement.
+/// instructions. Ignoring any access pattern would weaken enforcement. For
+/// example, AccessedStorageAnalysis cannot be used here because that analysis
+/// may conservatively summarize some functions.
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "access-enforcement-wmo"