Change the algorithm for the AccessEnforcementDom pass.

This adds a mostly flow-insensitive analysis that runs before the
dominator-based transformations. The analysis is simple and efficient
because it only needs to track data flow of currently in-scope
accesses. The original dominator tree walk remains simple, but it now
checks the flow insensitive analysis information to determine general
correctness. This is now correct in the presence of all kinds of nested
static and dynamic nested accesses, call sites, coroutines, etc.

This is a better compromise than:

(a) disabling the pass and taking a major performance loss.

(b) converting the pass itself to full-fledged data flow driven
optimization, which would be more optimal because it could remove
accesses when nesting is involved, but would be much more expensive
and complicated, and there's no indication that it's useful.

The new approach is also simpler than adding more complexity to
independently handle to each of many issues:

- Nested reads followed by a modify without a false conflict.
- Reads nested within a function call without a false conflict.
- Conflicts nested within a function call without dropping enforcement.
- Accesses within a generalized accessor.
- Conservative treatment of invalid storage locations.
- Conservative treatment of unknown apply callee.
- General analysis invalidation.

Some of these issues also needed to be considered in the
LoopDominatingAccess sub-pass. Rather than fix that sub-pass, I just
integrated it into the main pass. This is a simplification, is more
efficient, and also handles nested loops without creating more
redundant accesses. It is also generalized to:
- hoist non-uniquely identified accesses.
- Avoid unnecessarily promoting accesses inside the loop.

With this approach we can remove the scary warnings and caveats in the
comments.

While doing this I also took the opportunity to eliminate quadratic
behavior, make the domtree walk non-recursive, and eliminate cutoff
thresholds.

Note that simple nested dynamic reads to identical storage could very
easily be removed via separate logic, but it does not fit with the
dominator-based algorithm. For example, during the analysis phase, we
could simply mark the "fully nested" read scopes, then convert them to
[static] right after the analysis, removing them from the result
map. I didn't do this because I don't know if it happens in practice.

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -160,6 +160,11 @@ class AccessedStorage {
                                64 - NumAccessedStorageBits,
                                seenNestedConflict : 1,
                                beginAccessIndex : 63 - NumAccessedStorageBits);
+
+    // Define data flow bits for use in the AccessEnforcementDom pass. Each
+    // begin_access in the function is mapped to one instance of this subclass.
+    SWIFT_INLINE_BITFIELD(DomAccessedStorage, AccessedStorage, 1 + 1,
+                          isInner : 1, containsRead : 1);
   } Bits;
 
 private:
@@ -190,6 +195,10 @@ class AccessedStorage {
 
   Kind getKind() const { return static_cast<Kind>(Bits.AccessedStorage.Kind); }
 
+  // Clear any bits reserved for subclass data. Useful for up-casting back to
+  // the base class.
+  void resetSubclassData() { initKind(getKind()); }
+
   SILValue getValue() const {
     assert(getKind() != Argument && getKind() != Global && getKind() != Class);
     return value;
@@ -215,6 +224,10 @@ class AccessedStorage {
     return objProj;
   }
 
+  /// Return true if the given storage objects have identical storage locations.
+  ///
+  /// This compares only the AccessedStorage base class bits, ignoring the
+  /// subclass bits. It is used for hash lookup equality.
   bool hasIdenticalBase(const AccessedStorage &other) const {
     if (getKind() != other.getKind())
       return false;
@@ -233,7 +246,6 @@ class AccessedStorage {
     case Class:
       return objProj == other.objProj;
     }
-    llvm_unreachable("unhandled kind");
   }
 
   /// Return true if the storage is guaranteed local.
@@ -311,31 +323,6 @@ class AccessedStorage {
   bool operator!=(const AccessedStorage &) const = delete;
 };
 
-/// Return true if the given storage objects have identical storage locations.
-///
-/// This compares only the AccessedStorage base class bits, ignoring the
-/// subclass bits.
-inline bool accessingIdenticalLocations(AccessedStorage LHS,
-                                        AccessedStorage RHS) {
-  if (LHS.getKind() != RHS.getKind())
-    return false;
-
-  switch (LHS.getKind()) {
-  case swift::AccessedStorage::Box:
-  case swift::AccessedStorage::Stack:
-  case swift::AccessedStorage::Nested:
-  case swift::AccessedStorage::Yield:
-  case swift::AccessedStorage::Unidentified:
-    return LHS.getValue() == RHS.getValue();
-  case swift::AccessedStorage::Argument:
-    return LHS.getParamIndex() == RHS.getParamIndex();
-  case swift::AccessedStorage::Global:
-    return LHS.getGlobal() == RHS.getGlobal();
-  case swift::AccessedStorage::Class:
-    return LHS.getObjectProjection() == RHS.getObjectProjection();
-  }
-}
-
 template <class ImplTy, class ResultTy = void, typename... ArgTys>
 class AccessedStorageVisitor {
   ImplTy &asImpl() { return static_cast<ImplTy &>(*this); }
@@ -396,7 +383,7 @@ template <> struct DenseMapInfo<swift::AccessedStorage> {
   }
 
   static bool isEqual(swift::AccessedStorage LHS, swift::AccessedStorage RHS) {
-    return swift::accessingIdenticalLocations(LHS, RHS);
+    return LHS.hasIdenticalBase(RHS);
   }
 };
 

lib/SIL/MemAccessUtils.cpp

@@ -425,7 +425,7 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
     }
     // `storage` may still be invalid. If both `storage` and `result` are
     // invalid, this check passes, but we return an invalid storage below.
-    if (!accessingIdenticalLocations(storage.getValue(), result.getStorage()))
+    if (!storage.getValue().hasIdenticalBase(result.getStorage()))
       return AccessedStorage();
   }
   return storage.getValueOr(AccessedStorage());