Merge pull request swiftlang#33017 from atrick/really-add-class-tail-storage

 Add AccessedStorage::Tail access kind and remove more exclusivity checks.

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -189,7 +189,9 @@ namespace swift {
 /// the base of a formal access. It may be from a ref_tail_addr, undef, or some
 /// recognized memory initialization pattern. Unidentified valid storage cannot
 /// represent any arbitrary base address--it must at least been proven not to
-/// correspond to any class or global variable access.
+/// correspond to any class or global variable access, unless it's nested within
+/// another access to the same object. So, Unidentified can overlap with
+/// Class/Global access, but it cannot be the only formal access to that memory.
 ///
 /// An *invalid* AccessedStorage object is Unidentified and associated with an
 /// invalid SILValue. This signals that analysis has failed to recognize an
@@ -219,6 +221,7 @@ class AccessedStorage {
     Stack,
     Global,
     Class,
+    Tail,
     Argument,
     Yield,
     Nested,
@@ -228,10 +231,16 @@ class AccessedStorage {
 
   static const char *getKindName(Kind k);
 
-  /// Directly create an AccessedStorage for class property access.
+  // Give object tail storage a fake property index for convenience.
+  static constexpr unsigned TailIndex = ~0U;
+
+  /// Directly create an AccessedStorage for class or tail property access.
   static AccessedStorage forClass(SILValue object, unsigned propertyIndex) {
     AccessedStorage storage;
-    storage.initKind(Class, propertyIndex);
+    if (propertyIndex == TailIndex)
+      storage.initKind(Tail);
+    else
+      storage.initKind(Class, propertyIndex);
     storage.value = object;
     return storage;
   }
@@ -301,7 +310,8 @@ class AccessedStorage {
   union {
     // For non-class storage, 'value' is the access base. For class storage
     // 'value' is the object base, where the access base is the class' stored
-    // property.
+    // property. For tail storage 'value' is the object base and there is no
+    // value for the access base.
     SILValue value;
     SILGlobalVariable *global;
   };
@@ -334,7 +344,7 @@ class AccessedStorage {
   }
 
   SILValue getValue() const {
-    assert(getKind() != Global && getKind() != Class);
+    assert(getKind() != Global && getKind() != Class && getKind() != Tail);
     return value;
   }
 
@@ -354,7 +364,7 @@ class AccessedStorage {
   }
 
   SILValue getObject() const {
-    assert(getKind() == Class);
+    assert(getKind() == Class || getKind() == Tail);
     return value;
   }
   unsigned getPropertyIndex() const {
@@ -374,6 +384,7 @@ class AccessedStorage {
     switch (getKind()) {
     case Box:
     case Stack:
+    case Tail:
     case Argument:
     case Yield:
     case Nested:
@@ -396,6 +407,7 @@ class AccessedStorage {
       return true;
     case Global:
     case Class:
+    case Tail:
     case Argument:
     case Yield:
     case Nested:
@@ -405,6 +417,11 @@ class AccessedStorage {
     llvm_unreachable("unhandled kind");
   }
 
+  /// Return trye if the given access is guaranteed to be within a heap object.
+  bool isObjectAccess() const {
+    return getKind() == Class || getKind() == Tail;
+  }
+
   /// Return true if the given access is on a 'let' lvalue.
   bool isLetAccess(SILFunction *F) const;
 
@@ -420,6 +437,7 @@ class AccessedStorage {
     case Global:
       return true;
     case Class:
+    case Tail:
     case Argument:
     case Yield:
     case Nested:
@@ -455,20 +473,47 @@ class AccessedStorage {
   // Return true if this storage is guaranteed not to overlap with \p other's
   // storage.
   bool isDistinctFrom(const AccessedStorage &other) const {
-    if (isUniquelyIdentified() && other.isUniquelyIdentified()) {
-      return !hasIdenticalBase(other);
+    if (isUniquelyIdentified()) {
+      if (other.isUniquelyIdentified() && !hasIdenticalBase(other))
+        return true;
+
+      if (other.isObjectAccess())
+        return true;
+
+      // We currently assume that Unidentified storage may overlap with
+      // Box/Stack storage.
+      return false;
     }
-    if (getKind() != Class || other.getKind() != Class)
-      // At least one side is an Argument or Yield, or is unidentified.
+    if (other.isUniquelyIdentified())
+      return other.isDistinctFrom(*this);
+
+    // Neither storage is uniquely identified.
+    if (isObjectAccess()) {
+      if (other.isObjectAccess()) {
+        // Property access cannot overlap with Tail access.
+        if (getKind() != other.getKind())
+          return true;
+
+        // We could also check if the object types are distinct, but that only
+        // helps if we know the relationships between class types.
+        return getKind() == Class
+               && getPropertyIndex() != other.getPropertyIndex();
+      }
+      // Any type of nested/argument address may be within the same object.
+      //
+      // We also currently assume Unidentified access may be within an object
+      // purely to handle KeyPath accesses. The deriviation of the KeyPath
+      // address must separately appear to be a Class access so that all Class
+      // accesses are accounted for.
       return false;
-
-    // Classes are not uniquely identified by their base. However, if the
-    // underling objects have identical types and distinct property indices then
-    // they are distinct storage locations.
-    if (getObject()->getType() == other.getObject()->getType()
-        && getPropertyIndex() != other.getPropertyIndex()) {
-      return true;
     }
+    if (other.isObjectAccess())
+      return other.isDistinctFrom(*this);
+
+    // Neither storage is from a class or tail.
+    //
+    // Unidentified values may alias with each other or with any kind of
+    // nested/argument access.
     return false;
   }
 
@@ -530,10 +575,11 @@ template <> struct DenseMapInfo<swift::AccessedStorage> {
       return storage.getParamIndex();
     case swift::AccessedStorage::Global:
       return DenseMapInfo<void *>::getHashValue(storage.getGlobal());
-    case swift::AccessedStorage::Class: {
+    case swift::AccessedStorage::Class:
       return llvm::hash_combine(storage.getObject(),
                                 storage.getPropertyIndex());
-    }
+    case swift::AccessedStorage::Tail:
+      return DenseMapInfo<swift::SILValue>::getHashValue(storage.getObject());
     }
     llvm_unreachable("Unhandled AccessedStorageKind");
   }
@@ -673,6 +719,9 @@ class AccessUseDefChainVisitor {
   Result visitClassAccess(RefElementAddrInst *field) {
     return asImpl().visitBase(field, AccessedStorage::Class);
   }
+  Result visitTailAccess(RefTailAddrInst *tail) {
+    return asImpl().visitBase(tail, AccessedStorage::Tail);
+  }
   Result visitArgumentAccess(SILFunctionArgument *arg) {
     return asImpl().visitBase(arg, AccessedStorage::Argument);
   }
@@ -808,7 +857,7 @@ Result AccessUseDefChainVisitor<Impl, Result>::visit(SILValue sourceAddr) {
   // This is a valid address producer for nested @inout argument
   // access, but it is never used for formal access of identified objects.
   case ValueKind::RefTailAddrInst:
-    return asImpl().visitUnidentified(sourceAddr);
+    return asImpl().visitTailAccess(cast<RefTailAddrInst>(sourceAddr));
 
   // Inductive single-operand cases:
   // Look through address casts to find the source address.

include/swift/SILOptimizer/PassManager/Passes.def

@@ -68,8 +68,10 @@ PASS(CrossModuleSerializationSetup, "cross-module-serialization-setup",
      "Setup serialization flags for cross-module optimization")
 PASS(AccessSummaryDumper, "access-summary-dump",
      "Dump Address Parameter Access Summary")
+PASS(AccessedStorageAnalysisDumper, "accessed-storage-analysis-dump",
+     "Dump Accessed Storage Analysis Summaries")
 PASS(AccessedStorageDumper, "accessed-storage-dump",
-     "Dump Accessed Storage Summary")
+     "Dump Accessed Storage Information")
 PASS(AccessMarkerElimination, "access-marker-elim",
      "Access Marker Elimination.")
 PASS(AddressLowering, "address-lowering",

lib/SIL/Utils/MemAccessUtils.cpp

@@ -119,6 +119,12 @@ AccessedStorage::AccessedStorage(SILValue base, Kind kind) {
     auto *REA = cast<RefElementAddrInst>(base);
     value = stripBorrow(REA->getOperand());
     setElementIndex(REA->getFieldNo());
+    break;
+  }
+  case Tail: {
+    auto *RTA = cast<RefTailAddrInst>(base);
+    value = stripBorrow(RTA->getOperand());
+    break;
   }
   }
 }
@@ -152,6 +158,9 @@ const ValueDecl *AccessedStorage::getDecl() const {
     auto *decl = getObject()->getType().getNominalOrBoundGenericNominal();
     return decl->getStoredProperties()[getPropertyIndex()];
   }
+  case Tail:
+    return nullptr;
+
   case Argument:
     return getArgument()->getDecl();
 
@@ -185,11 +194,17 @@ const char *AccessedStorage::getKindName(AccessedStorage::Kind k) {
     return "Global";
   case Class:
     return "Class";
+  case Tail:
+    return "Tail";
   }
   llvm_unreachable("unhandled kind");
 }
 
 void AccessedStorage::print(raw_ostream &os) const {
+  if (!*this) {
+    os << "INVALID\n";
+    return;
+  }
   os << getKindName(getKind()) << " ";
   switch (getKind()) {
   case Box:
@@ -211,6 +226,9 @@ void AccessedStorage::print(raw_ostream &os) const {
     getDecl()->print(os);
     os << " Index: " << getPropertyIndex() << "\n";
     break;
+  case Tail:
+    os << getObject();
+    os << "  Tail\n";
   }
 }
 
@@ -246,10 +264,16 @@ class FindPhiStorageVisitor
       this->visit(pointerWorklist.pop_back_val());
     }
     // If a common path component was found, recursively look for the storage.
-    if (commonDefinition && commonDefinition.getValue()) {
-      auto storage = storageVisitor.findStorage(commonDefinition.getValue());
-      (void)storage; // The same storageVisitor called us. It has already
-                     // recorded the storage that it found.
+    if (commonDefinition) {
+      if (commonDefinition.getValue()) {
+        auto storage = storageVisitor.findStorage(commonDefinition.getValue());
+        (void)storage; // The same storageVisitor called us. It has already
+                       // recorded the storage that it found.
+      } else {
+        // If divergent paths were found, invalidate any previously discovered
+        // storage.
+        storageVisitor.setStorage(AccessedStorage());
+      }
     }
   }
 
@@ -568,6 +592,9 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
     break;
   case AccessedStorage::Class:
     break;
+  case AccessedStorage::Tail:
+    return false;
+
   case AccessedStorage::Yield:
     // Yields are accessed by the caller.
     return false;

lib/SIL/Verifier/LoadBorrowInvalidationChecker.cpp

@@ -486,6 +486,11 @@ bool LoadBorrowNeverInvalidatedAnalysis::isNeverInvalidated(
   // validate that all uses of the project_box are not writes that overlap with
   // our load_borrow's result. These are things that may not be a formal access
   // base.
+  //
+  // FIXME: we don't seem to verify anywhere that a pointer_to_address cannot
+  // itself be derived from another address that is accessible in the same
+  // function, either because it was returned from a call or directly
+  // address_to_pointer'd.
   if (isa<PointerToAddressInst>(address)) {
     return doesAddressHaveWriteThatInvalidatesLoadBorrow(lbi, endBorrowUses,
                                                          address);
@@ -525,6 +530,9 @@ bool LoadBorrowNeverInvalidatedAnalysis::isNeverInvalidated(
   }
   case AccessedStorage::Yield: {
     // For now, do this. Otherwise, check for in_guaranteed, etc.
+    //
+    // FIXME: The yielded address could overlap with another address in this
+    // function.
     return true;
   }
   case AccessedStorage::Box: {
@@ -534,9 +542,18 @@ bool LoadBorrowNeverInvalidatedAnalysis::isNeverInvalidated(
   case AccessedStorage::Class: {
     // Check that the write to the class's memory doesn't overlap with our
     // load_borrow.
+    //
+    // FIXME: how do we know that other projections of the same object don't
+    // occur within the same function?
     return doesAddressHaveWriteThatInvalidatesLoadBorrow(lbi, endBorrowUses,
                                                          address);
   }
+  case AccessedStorage::Tail: {
+    // This should be as strong as the Class address case, but to handle it we
+    // need to find all aliases of the object and all tail projections within
+    // that object.
+    return false;
+  }
   case AccessedStorage::Global: {
     // Check that the write to the class's memory doesn't overlap with our
     // load_borrow.

lib/SILOptimizer/Analysis/AccessedStorageAnalysis.cpp

@@ -230,14 +230,17 @@ transformCalleeStorage(const StorageAccessInfo &storage,
   case AccessedStorage::Global:
     // Global accesses is universal.
     return storage;
-  case AccessedStorage::Class: {
+  case AccessedStorage::Class:
+  case AccessedStorage::Tail: {
     // If the object's value is an argument, translate it into a value on the
     // caller side.
     SILValue obj = storage.getObject();
     if (auto *arg = dyn_cast<SILFunctionArgument>(obj)) {
       SILValue argVal = getCallerArg(fullApply, arg->getIndex());
       if (argVal) {
-        unsigned idx = storage.getPropertyIndex();
+        unsigned idx = (storage.getKind() == AccessedStorage::Class)
+                           ? storage.getPropertyIndex()
+                           : AccessedStorage::TailIndex;
         // Remap this storage info. The argument source value is now the new
         // object. The old storage info is inherited.
         return StorageAccessInfo(AccessedStorage::forClass(argVal, idx),

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

@@ -743,6 +743,7 @@ void AccessConflictAndMergeAnalysis::visitMayRelease(SILInstruction *instr,
   // accesses can be affected by a deinitializer.
   auto isHeapAccess = [](AccessedStorage::Kind accessKind) {
     return accessKind == AccessedStorage::Class
+           || accessKind == AccessedStorage::Class
            || accessKind == AccessedStorage::Global;
   };
   // Mark the in-scope accesses as having a nested conflict

lib/SILOptimizer/Transforms/AccessEnforcementWMO.cpp

@@ -85,6 +85,7 @@ const VarDecl *getDisjointAccessLocation(const AccessedStorage &storage) {
   }
   case AccessedStorage::Box:
   case AccessedStorage::Stack:
+  case AccessedStorage::Tail:
   case AccessedStorage::Argument:
   case AccessedStorage::Yield:
   case AccessedStorage::Unidentified: