MemBehavior: fix minor bugs for begin_access and end_access.

* ``begin_access [modify]`` returned MayWrite, but "modify" means, it can be a read as well.
Instead, return MayReadWrite. Only for ``begin_access [init]`` return MayWrite.
This is more or less cosmetic - probably this bug had no real impact on any optimization.

* ``begin_access [deinit]`` needs to return MayReadWrite for the same reason ``destroy_addr`` returns MayReadWrite (see SILInstruction::MemoryBehavior).

Author: eeckstein

lib/SILOptimizer/Analysis/MemoryBehavior.cpp

@@ -136,37 +136,38 @@ class MemoryBehaviorVisitor
   }
 
   MemBehavior visitBeginAccessInst(BeginAccessInst *beginAccess) {
+    if (!mayAlias(beginAccess->getSource()))
+      return MemBehavior::None;
+
+    // begin_access does not physically read or write memory. But we model it
+    // as a memory read and/or write to prevent optimizations to move other
+    // aliased loads/stores across begin_access into the access scope.
     switch (beginAccess->getAccessKind()) {
     case SILAccessKind::Deinit:
-      // 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;
+      // For the same reason we treat a ``load [take]`` or a ``destroy_addr``
+      // as a memory write, we do that for a ``begin_access [deinit]`` as well.
+      // See SILInstruction::MemoryBehavior.
+      return MemBehavior::MayReadWrite;
     case SILAccessKind::Read:
-      if (!mayAlias(beginAccess->getSource()))
-        return MemBehavior::None;
-
       return MemBehavior::MayRead;
-
     case SILAccessKind::Modify:
       if (isLetValue()) {
         assert(getAccessBase(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;
+      return MemBehavior::MayReadWrite;
     case SILAccessKind::Init:
-      if (!mayAlias(beginAccess->getSource()))
-        return MemBehavior::None;
-
       return MemBehavior::MayWrite;
     }
     llvm_unreachable("invalid access kind");
   }
 
   MemBehavior visitEndAccessInst(EndAccessInst *endAccess) {
+    // end_access does not physically read or write memory. But, similar to
+    // begin_access, we model it as a memory read and/or write to prevent
+    // optimizations to move other aliased loads/stores across end_access into
+    // the access scope.
     return visitBeginAccessInst(endAccess->getBeginAccess());
   }
 

test/SILOptimizer/mem-behavior.sil

@@ -491,23 +491,23 @@ bb0(%0 : @owned $Parent, %1 : $*Builtin.Int32):
 // CHECK: PAIR #8.
 // CHECK-NEXT:    %{{.*}} = begin_access [modify] [static] %0 : $*Builtin.Int32
 // CHECK-NEXT:    %0 = argument of bb0 : $*Builtin.Int32
-// CHECK-NEXT:  r=0,w=1
+// CHECK-NEXT:  r=1,w=1
 // CHECK: PAIR #9.
 // CHECK-NEXT:    %{{.*}} = begin_access [modify] [static] %0 : $*Builtin.Int32
 // CHECK-NEXT:    %{{.*}} = begin_access [read] [static] %0 : $*Builtin.Int32
-// CHECK-NEXT:  r=0,w=1
+// CHECK-NEXT:  r=1,w=1
 // CHECK: PAIR #13.
 // CHECK-NEXT:    end_access %{{.*}} : $*Builtin.Int32
 // CHECK-NEXT:    %0 = argument of bb0 : $*Builtin.Int32
-// CHECK-NEXT:  r=0,w=1
+// CHECK-NEXT:  r=1,w=1
 // CHECK: PAIR #14.
 // CHECK-NEXT:    end_access %{{.*}} : $*Builtin.Int32
 // CHECK-NEXT:    %{{.*}} = begin_access [read] [static] %0 : $*Builtin.Int32
-// CHECK-NEXT:  r=0,w=1
+// CHECK-NEXT:  r=1,w=1
 // CHECK: PAIR #15.
 // CHECK-NEXT:    end_access %{{.*}} : $*Builtin.Int32
 // CHECK-NEXT:    %{{.*}} = begin_access [modify] [static] %0 : $*Builtin.Int32
-// CHECK-NEXT:  r=0,w=1
+// CHECK-NEXT:  r=1,w=1
 sil [ossa] @testReadWriteAccess : $@convention(thin) (@inout Builtin.Int32) -> Builtin.Int32 {
 bb0(%0 : $*Builtin.Int32):
   %read = begin_access [read] [static] %0 : $*Builtin.Int32
@@ -520,6 +520,42 @@ bb0(%0 : $*Builtin.Int32):
   return %val : $Builtin.Int32
 }
 
+// CHECK-LABEL: @testDeinitAccess
+// CHECK:      PAIR #0.
+// CHECK-NEXT:     %1 = begin_access [deinit] [static] %0 : $*Builtin.Int32
+// CHECK-NEXT:     %0 = argument of bb0 : $*Builtin.Int32
+// CHECK-NEXT:   r=1,w=1
+// CHECK:      PAIR #1.
+// CHECK-NEXT:    end_access %1 : $*Builtin.Int32
+// CHECK-NEXT:    %0 = argument of bb0 : $*Builtin.Int32
+// CHECK-NEXT:   r=1,w=1
+sil [ossa] @testDeinitAccess : $@convention(thin) (@in Builtin.Int32) -> () {
+bb0(%0 : $*Builtin.Int32):
+  %1 = begin_access [deinit] [static] %0 : $*Builtin.Int32
+  end_access %1 : $*Builtin.Int32
+  %r = tuple ()
+  return %r : $()
+}
+
+// CHECK-LABEL: @testInitAccess
+// CHECK: PAIR #0.
+// CHECK-NEXT:     %{{.*}} = begin_access [init] [static] %0 : $*Builtin.Int32
+// CHECK-NEXT:     %0 = argument of bb0 : $*Builtin.Int32
+// CHECK-NEXT:   r=0,w=1
+// CHECK: PAIR #3.
+// CHECK-NEXT:    end_access %{{.*}} : $*Builtin.Int32
+// CHECK-NEXT:    %0 = argument of bb0 : $*Builtin.Int32
+// CHECK-NEXT:  r=0,w=1
+sil [ossa] @testInitAccess : $@convention(thin) (Builtin.Int32) -> @out Builtin.Int32 {
+bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32):
+  %init = begin_access [init] [static] %0 : $*Builtin.Int32
+  store %1 to [trivial] %init : $*Builtin.Int32
+  end_access %init : $*Builtin.Int32
+  %val = load [trivial] %0 : $*Builtin.Int32
+  %r = tuple ()
+  return %r : $()
+}
+
 // CHECK-LABEL: @testLoadTake
 // CHECK:      PAIR #0.
 // CHECK-NEXT:   %2 = load [take] %0 : $*C
@@ -572,7 +608,7 @@ struct Int64Wrapper {
 // CHECK: PAIR #2.
 // CHECK:     %1 = begin_access [modify] [static] %0 : $*Int64Wrapper // users: %7, %2
 // CHECK:     %3 = begin_access [read] [static] %2 : $*Int64  // users: %6, %4
-// CHECK:   r=0,w=1
+// CHECK:   r=1,w=1
 // CHECK: PAIR #3.
 sil @testNestedAccessWithInterposedProjection : $@convention(thin) (@inout Int64Wrapper) -> () {
 bb0(%0 : $*Int64Wrapper):