Reduce compile time for large strongly connected call graphs.

Improves SwiftSyntax release build speed by 4x.

Limit the size of the sets tracked by inter procedural
AccessedStorageAnalysis. There is a lot of leeway in this limit since
"normal" code doesn't come close to hitting it and SwiftSyntax compile
time isn't noticeably affected until 10x this limit.

This change also avoids reanalyzing function bodies once results have
bottomed out and avoids copying sets in the common case.

Fixes <rdar://problem/46905624> Release build time regression, a lot of time spent on AccessEnforcementOpts::run().

This is just a band aid. The fundamental problem is really:
<rdar://problem/47195282> Recomputing BottomUpIPAnalysis takes most of
the SwiftSyntax compile time.

SwiftSyntax compile time is still at least an order of magnitude
longer than it should be.

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"