TempRValueOpt: small fixes/improvements for extending access scopes

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());
   }
 

lib/SILOptimizer/Transforms/TempRValueElimination.cpp

@@ -367,19 +367,27 @@ bool TempRValueOptPass::extendAccessScopes(
 
   for (SILInstruction &inst : make_range(begin, end)) {
     if (auto *endAccess = dyn_cast<EndAccessInst>(&inst)) {
+      // To keep things simple, we can just move a single end_access. Also, we
+      // cannot move an end_access over a (non-aliasing) end_access.
+      if (endAccessToMove)
+        return false;
       // Is this the end of an access scope of the copy-source?
       if (!aa->isNoAlias(copySrc, endAccess->getSource())) {
-        // To keep things simple, we can just move a single end_access.
-        if (endAccessToMove)
-          return false;
+        assert(endAccess->getBeginAccess()->getAccessKind() ==
+                 SILAccessKind::Read &&
+               "a may-write end_access should not be in the copysrc lifetime");
         endAccessToMove = endAccess;
       }
     } else if (endAccessToMove) {
       // We cannot move an end_access over a begin_access. This would destroy
       // the proper nesting of accesses.
-      if (isa<BeginAccessInst>(&inst))
+      if (isa<BeginAccessInst>(&inst) || isa<BeginUnpairedAccessInst>(inst))
         return false;
       // Don't extend a read-access scope over a (potential) write.
+      // Note that inst can be a function call containing other access scopes.
+      // But doing the mayWriteToMemory check, we know that the function can
+      // only contain read accesses (to the same memory location). So it's fine
+      // to move endAccessToMove even over such a function call.
       if (aa->mayWriteToMemory(&inst, endAccessToMove->getSource()))
         return false;
     }

lib/SILOptimizer/UtilityPasses/MemBehaviorDumper.cpp

@@ -24,10 +24,6 @@
 
 using namespace swift;
 
-static llvm::cl::opt<bool> EnableDumpAll(
-    "enable-mem-behavior-dump-all", llvm::cl::init(false),
-    llvm::cl::desc("With -mem-behavior-dump, dump all memory access pairs."));
-
 //===----------------------------------------------------------------------===//
 //                               Value Gatherer
 //===----------------------------------------------------------------------===//
@@ -58,14 +54,25 @@ class MemBehaviorDumper : public SILModuleTransform {
   // To reduce the amount of output, we only dump the memory behavior of
   // selected types of instructions.
   static bool shouldTestInstruction(SILInstruction *I) {
-    // Only consider function calls.
-    if ((EnableDumpAll && I->mayReadOrWriteMemory()) ||
-        FullApplySite::isa(I) ||
-        isa<EndApplyInst>(I) ||
-        isa<AbortApplyInst>(I))
+    switch (I->getKind()) {
+    case SILInstructionKind::ApplyInst:
+    case SILInstructionKind::TryApplyInst:
+    case SILInstructionKind::EndApplyInst:
+    case SILInstructionKind::BeginApplyInst:
+    case SILInstructionKind::AbortApplyInst:
+    case SILInstructionKind::BeginAccessInst:
+    case SILInstructionKind::EndAccessInst:
+    case SILInstructionKind::EndCOWMutationInst:
+    case SILInstructionKind::CopyValueInst:
+    case SILInstructionKind::DestroyValueInst:
+    case SILInstructionKind::EndBorrowInst:
+    case SILInstructionKind::LoadInst:
+    case SILInstructionKind::StoreInst:
+    case SILInstructionKind::CopyAddrInst:
       return true;
-
-    return false;
+    default:
+      return false;
+    }
   }
 
   void run() override {
@@ -88,6 +95,9 @@ class MemBehaviorDumper : public SILModuleTransform {
             for (auto &V : Values) {
               if (V->getDefiningInstruction() == &I)
                 continue;
+
+              if (!V->getType().isAddress() && !isa<AddressToPointerInst>(V))
+                continue;
 
               bool Read = AA->mayReadFromMemory(&I, V);
               bool Write = AA->mayWriteToMemory(&I, V);