Fix MemBehavior calls to getAccessedAddress to avoid an assert.

Fixes <rdar://60046018> assert: (v->getType().isAddress())
in getAccessedAddress.

MemBehavior compares known memory accesses with some arbitrary value,
which may not be an address. However, we should not call utilities
that work on accessed addresses with an arbitrary value.

This assert is a result of very recent changes to gradually make
memory access utilities more type safe and introduce the concept of a
canonical accessed address. In the future, we may even have a wrapper
type for such a thing. In the SIL optimizer, there are several
different notions of what constitutes the base of a memory
access. Mismatches can lead to subtle bugs.

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 : $()
+}