Merge pull request swiftlang#30369 from atrick/fix-verify-exclusivity

Fix MemBehavior calls to getAccessedAddress to avoid an assert.

Author: atrick

lib/SILOptimizer/Analysis/MemoryBehavior.cpp

@@ -56,6 +56,11 @@ class MemoryBehaviorVisitor
   /// The value we are attempting to discover memory behavior relative to.
   SILValue V;
 
+  /// Cache either the address of the access corresponding to memory at 'V', or
+  /// 'V' itself if it isn't recognized as part of an access. The cached value
+  /// is always a valid SILValue.
+  SILValue cachedValueAddress;
+
   Optional<bool> cachedIsLetValue;
 
   /// The SILType of the value.
@@ -77,11 +82,21 @@ class MemoryBehaviorVisitor
     return *TypedAccessTy;
   }
 
+  /// If 'V' is an address projection within a formal access, return the
+  /// canonical address of the formal access. Otherwise, return 'V' itself,
+  /// which is either a reference or unknown pointer or address.
+  SILValue getValueAddress() {
+    if (!cachedValueAddress) {
+      cachedValueAddress = V->getType().isAddress() ? getAccessedAddress(V) : V;
+    }
+    return cachedValueAddress;
+  }
+
   /// Return true if 'V's accessed address is that of a let variables.
   bool isLetValue() {
     if (!cachedIsLetValue) {
       cachedIsLetValue =
-          V->getType().isAddress() && isLetAddress(getAccessedAddress(V));
+          V->getType().isAddress() && isLetAddress(getValueAddress());
     }
     return cachedIsLetValue.getValue();
   }
@@ -124,15 +139,11 @@ class MemoryBehaviorVisitor
 
   MemBehavior visitBeginAccessInst(BeginAccessInst *beginAccess) {
     switch (beginAccess->getAccessKind()) {
-    case SILAccessKind::Init:
     case SILAccessKind::Deinit:
-      // No address aliasing is expected for variable initialization or
-      // deinitialization.
-      if (stripAccessMarkers(beginAccess) != getAccessedAddress(V))
-        return MemBehavior::None;
-
-      return MemBehavior::MayWrite;
-
+      // A [deinit] only directly reads from the object. The fact that it frees
+      // memory is modeled more precisely by the release operations within the
+      // deinit scope. Therefore, handle it like a [read] here...
+      LLVM_FALLTHROUGH;
     case SILAccessKind::Read:
       if (!mayAlias(beginAccess->getSource()))
         return MemBehavior::None;
@@ -141,10 +152,14 @@ class MemoryBehaviorVisitor
 
     case SILAccessKind::Modify:
       if (isLetValue()) {
-        assert(stripAccessMarkers(beginAccess) != getAccessedAddress(V)
+        assert(stripAccessMarkers(beginAccess) != getValueAddress()
                && "let modification not allowed");
         return MemBehavior::None;
       }
+      // [modify] has a special case for ignoring 'let's, but otherwise is
+      // identical to an [init]...
+      LLVM_FALLTHROUGH;
+    case SILAccessKind::Init:
       if (!mayAlias(beginAccess->getSource()))
         return MemBehavior::None;
 
@@ -229,15 +244,15 @@ MemBehavior MemoryBehaviorVisitor::visitLoadInst(LoadInst *LI) {
     return MemBehavior::None;
 
   LLVM_DEBUG(llvm::dbgs() << "  Could not prove that load inst does not alias "
-                             "pointer. Returning may read.");
+                             "pointer. Returning may read.\n");
   return MemBehavior::MayRead;
 }
 
 MemBehavior MemoryBehaviorVisitor::visitStoreInst(StoreInst *SI) {
   // No store besides the initialization of a "let"-variable
   // can have any effect on the value of this "let" variable.
   if (isLetValue()
-      && (getAccessedAddress(SI->getDest()) != getAccessedAddress(V))) {
+      && (getAccessedAddress(SI->getDest()) != getValueAddress())) {
     return MemBehavior::None;
   }
   // If the store dest cannot alias the pointer in question, then the

test/SILOptimizer/access_marker_verify.swift

@@ -1080,3 +1080,27 @@ public func getStaticProp() -> HasStaticProp {
 // CHECK: [[ADR:%.*]] = pointer_to_address [[RP]] : $Builtin.RawPointer to [strict] $*HasStaticProp
 // CHECK: load [copy] [[ADR]] : $*HasStaticProp
 // CHECK-LABEL: } // end sil function '$s20access_marker_verify13getStaticPropAA03HaseF0CyF'
+
+// ===-----------------------------------------------------------------------===
+// Test computeMemBehavior with a non-address value. In this case, the
+// address for an access of StreamClass.buffer is compared with the
+// ContiguousArrayStorageBase value.
+//
+// <rdar://60046018> assert: (v->getType().isAddress()) in getAccessedAddress.
+
+public final class StreamClass {
+    private var buffer: [UInt8]
+
+    public init() {
+        self.buffer = []
+    }
+}
+
+// CHECK-LABEL: sil [ossa] @$s20access_marker_verify25testNonAddressMemBehavioryySiF : $@convention(thin) (Int) -> () {
+// The relevant SIL instructions for this test don't appear until the string interpolation is inlined.
+// Just make sure it does not assert.
+// CHECK-LABEL: } // end sil function '$s20access_marker_verify25testNonAddressMemBehavioryySiF'
+public func testNonAddressMemBehavior(_ N: Int) {
+  let listOfStrings: [String] = (0..<10).map { "This is the number: \($0)!\n" }
+  let stream = StreamClass()
+}

test/SILOptimizer/mem-behavior-all.sil

@@ -112,3 +112,32 @@ bb0(%0 : $*Builtin.Int32):
   end_access %write : $*Builtin.Int32
   return %val : $Builtin.Int32
 }
+
+// ===-----------------------------------------------------------------------===
+// Test the effect of a [deinit] access on a global 'let'
+//
+// Test <rdar://60046018> Assert: (v->getType().isAddress()), getAccessedAddress
+
+sil_global hidden [let] @globalC : $C
+
+// CHECK-LABEL: @testDeinitInstVsNonAddressValue
+// CHECK:      PAIR #5.
+// CHECK-NEXT:     load %{{.*}} : $*C
+// CHECK-NEXT:     begin_access [deinit] [static] %{{.*}} : $*Builtin.Int32
+// CHECK-NEXT:   r=1,w=0,se=0
+// CHECK:      PAIR #16.
+// CHECK-NEXT:    %6 = begin_access [deinit] [static] %5 : $*Builtin.Int32 // user: %7
+// CHECK-NEXT:    %2 = load %1 : $*C                              // user: %3
+// CHECK-NEXT:  r=1,w=0,se=0
+sil hidden @testDeinitInstVsNonAddressValue : $@convention(thin) (@guaranteed C) -> () {
+bb0(%0 : $C):
+  %1 = global_addr @globalC : $*C
+  %2 = load %1 : $*C
+  %3 = ref_element_addr %2 : $C, #C.prop
+  %4 = load %3 : $*Builtin.Int32
+  %5 = ref_element_addr %0 : $C, #C.prop
+  %6 = begin_access [deinit] [static] %5 : $*Builtin.Int32
+  end_access %6 : $*Builtin.Int32
+  %8 = tuple ()
+  return %8 : $()
+}