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

Distinguish ref_tail_addr storage from the other storage classes.

We didn't have this originally because be don't expect a begin_access
to directly operate on tail storage. It could occur after inlining, at
least with static access markers. More importantly it helps ditinguish
regular formal accesses from other unidentified access, so we probably
should have always had this.

At any rate, it's particularly important when AccessedStorage is
generalized to arbitrary memory access.

The immediate motivation is to add an AccessPath utility, which will
need to distinguish tail storage.

In the process, rewrite AccessedStorage::isDistinct. This could have a
large positive impact on exclusivity performance.

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.

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,6 +194,8 @@ const char *AccessedStorage::getKindName(AccessedStorage::Kind k) {
     return "Global";
   case Class:
     return "Class";
+  case Tail:
+    return "Tail";
   }
   llvm_unreachable("unhandled kind");
 }
@@ -215,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";
   }
 }
 
@@ -578,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:

test/SILOptimizer/accessed_storage.sil

@@ -123,9 +123,9 @@ struct MyArray<T> {
 
 // CHECK-LABEL: @arrayValue
 // CHECK:   load [trivial] %{{.*}} : $*Builtin.Int64
-// CHECK: Unidentified  %{{.*}} = ref_tail_addr [immutable] [[REF:%[0-9]+]] : $__ContiguousArrayStorageBase, $Int64
+// CHECK: Tail  %{{.*}} = unchecked_ref_cast [[REF:%[0-9]+]] : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
 // CHECK:   load [trivial] %{{.*}} : $*Builtin.Int64
-// CHECK: Unidentified  %{{.*}} = ref_tail_addr [immutable] [[REF]] : $__ContiguousArrayStorageBase, $Int64
+// CHECK: Tail  %{{.*}} = unchecked_ref_cast [[REF:%[0-9]+]] : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
 sil [ossa] @arrayValue : $@convention(thin) (@guaranteed MyArray<Int64>) -> Int64 {
 bb0(%0 : @guaranteed $MyArray<Int64>):
   %1 = integer_literal $Builtin.Word, 3

test/SILOptimizer/optimize_keypath.swift

@@ -561,7 +561,7 @@ func testGetOptionalForceClass(_ s: SimpleClass) -> Int {
 // CHECK-LABEL: sil {{.*}}testGetComplex
 // opt
 // CHECK: [[E1:%[0-9]+]] = ref_element_addr
-// CHECK: [[B1:%[0-9]+]] = begin_access [read] [dynamic] [[E1]]
+// CHECK: [[B1:%[0-9]+]] = begin_access [read] [dynamic] [no_nested_conflict] [[E1]]
 // !
 // CHECK: [[F:%[0-9]+]] = select_enum [[O:%[0-9]+]]
 // CHECK: cond_fail [[F]]