[LICM] Refactoring + Improvements + Exclusivity Support

Major refactoring + tuning of LICM. Includes:
Support for hosting more array semantic calls
Remove restrictions for sinking instructions
Add support for hoisting and sinking instruction pairs (begin and end accesses)

Testing with Exclusivity enabled on a couple of benchmarks shows:
ReversedArray 7x improvement
StringWalk 2.6x improvement

lib/SILOptimizer/LoopTransforms/LICM.cpp

@@ -13,21 +13,22 @@
 #define DEBUG_TYPE "sil-licm"
 
 #include "swift/SIL/Dominance.h"
+#include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/MemAccessUtils.h"
+#include "swift/SIL/SILArgument.h"
+#include "swift/SIL/SILBuilder.h"
+#include "swift/SIL/SILInstruction.h"
 #include "swift/SILOptimizer/Analysis/AliasAnalysis.h"
 #include "swift/SILOptimizer/Analysis/Analysis.h"
+#include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
 #include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
-#include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SILOptimizer/Analysis/SideEffectAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/CFG.h"
-#include "swift/SILOptimizer/Utils/SILSSAUpdater.h"
 #include "swift/SILOptimizer/Utils/Local.h"
-#include "swift/SIL/SILArgument.h"
+#include "swift/SILOptimizer/Utils/SILSSAUpdater.h"
-#include "swift/SIL/SILBuilder.h"
-#include "swift/SIL/SILInstruction.h"
-#include "swift/SIL/InstructionUtils.h"
 
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -37,19 +38,27 @@
 
 using namespace swift;
 
-/// Instructions which read from memory, e.g. loads, or function calls without
+/// Instructions which can be hoisted:
-/// side effects.
+/// loads, function calls without side effects and (some) exclusivity checks
-using ReadSet = llvm::SmallPtrSet<SILInstruction *, 8>;
+using HoistSet = llvm::SmallPtrSet<SILInstruction *, 8>;
 
-/// Instructions which (potentially) write memory.
+/// Instruction pairs which need to be hoisted together:
-using WriteSet = SmallVector<SILInstruction *, 8>;
+/// e.g. If we hoist a begin access, we need to sink the matching end access
+using HoistPairSet =
+    llvm::SmallVector<std::pair<SILInstruction *, SILInstruction *>, 8>;
+
+/// A subset of instruction which may have side effects.
+/// Doesn't contain ones that have special handling (e.g. fix_lifetime)
+using WriteSet = SmallPtrSet<SILInstruction *, 8>;
 
 /// Returns true if the \p MayWrites set contains any memory writes which may
 /// alias with the memory addressed by \a LI.
-static bool mayWriteTo(AliasAnalysis *AA, WriteSet &MayWrites, LoadInst *LI) {
+template <SILInstructionKind K, typename T>
+static bool mayWriteTo(AliasAnalysis *AA, WriteSet &MayWrites,
+                       UnaryInstructionBase<K, T> *Inst) {
   for (auto *W : MayWrites)
-    if (AA->mayWriteToMemory(W, LI->getOperand())) {
+    if (AA->mayWriteToMemory(W, Inst->getOperand())) {
-      DEBUG(llvm::dbgs() << "  mayWriteTo\n" << *W << " to " << *LI << "\n");
+      DEBUG(llvm::dbgs() << "  mayWriteTo\n" << *W << " to " << *Inst << "\n");
       return true;
     }
   return false;
@@ -57,6 +66,7 @@ static bool mayWriteTo(AliasAnalysis *AA, WriteSet &MayWrites, LoadInst *LI) {
 
 /// Returns true if the \p MayWrites set contains any memory writes which may
 /// alias with any memory which is read by \p AI.
+/// Note: This function should only be called on a read-only apply!
 static bool mayWriteTo(AliasAnalysis *AA, SideEffectAnalysis *SEA,
                        WriteSet &MayWrites, ApplyInst *AI) {
   FunctionSideEffects E;
@@ -64,11 +74,8 @@ static bool mayWriteTo(AliasAnalysis *AA, SideEffectAnalysis *SEA,
   assert(E.getMemBehavior(RetainObserveKind::IgnoreRetains) <=
          SILInstruction::MemoryBehavior::MayRead &&
          "apply should only read from memory");
-  if (E.getGlobalEffects().mayRead() && !MayWrites.empty()) {
+  assert(!E.getGlobalEffects().mayRead() &&
-    // We don't know which memory is read in the callee. Therefore we bail if
+         "apply should not have global effects");
-    // there are any writes in the loop.
-    return true;
-  }
 
   for (unsigned Idx = 0, End = AI->getNumArguments(); Idx < End; ++Idx) {
     auto &ArgEffect = E.getParameterEffects()[Idx];
@@ -89,25 +96,6 @@ static bool mayWriteTo(AliasAnalysis *AA, SideEffectAnalysis *SEA,
   return false;
 }
 
-static void removeWrittenTo(AliasAnalysis *AA, ReadSet &Reads,
-                            SILInstruction *ByInst) {
-
-  // We can ignore retains, cond_fails, and dealloc_stacks.
-  if (isa<StrongRetainInst>(ByInst) || isa<RetainValueInst>(ByInst) ||
-      isa<CondFailInst>(ByInst) || isa<DeallocStackInst>(ByInst))
-    return;
-
-  SmallVector<SILInstruction *, 8> RS(Reads.begin(), Reads.end());
-  for (auto R : RS) {
-    auto *LI = dyn_cast<LoadInst>(R);
-    if (LI && !AA->mayWriteToMemory(ByInst, LI->getOperand()))
-      continue;
-
-    DEBUG(llvm::dbgs() << "  mayWriteTo\n" << *ByInst << " to " << *R << "\n");
-    Reads.erase(R);
-  }
-}
-
 static bool hasLoopInvariantOperands(SILInstruction *I, SILLoop *L) {
   auto Opds = I->getAllOperands();
 
@@ -125,64 +113,12 @@ static bool hasLoopInvariantOperands(SILInstruction *I, SILLoop *L) {
   });
 }
 
-/// Checks if \p Inst has no side effects which prevent hoisting.
+// When Hoisting / Sinking,
-/// The \a SafeReads set contain instructions which we already proved to have
+// Don't descend into control-dependent code.
-/// no such side effects.
+// Only traverse into basic blocks that dominate all exits.
-static bool hasNoSideEffect(SILInstruction *Inst, ReadSet &SafeReads) {
+static void getDominatingBlocks(SmallVectorImpl<SILBasicBlock *> &domBlocks,
-  // We can (and must) hoist cond_fail instructions if the operand is
+                                SILLoop *Loop, DominanceInfo *DT) {
-  // invariant. We must hoist them so that we preserve memory safety. A
-  // cond_fail that would have protected (executed before) a memory access
-  // must - after hoisting - also be executed before said access.
-  if (isa<CondFailInst>(Inst))
-    return true;
-  
-  // Can't hoist if the instruction could read from memory and is not marked
-  // as safe.
-  if (SafeReads.count(Inst))
-    return true;
-
-  if (Inst->getMemoryBehavior() == SILInstruction::MemoryBehavior::None)
-    return true;
-  
-  return false;
-}
-
-static bool canHoistInstruction(SILInstruction *Inst, SILLoop *Loop,
-                                ReadSet &SafeReads) {
-  // Can't hoist terminators.
-  if (isa<TermInst>(Inst))
-    return false;
-  
-  // Can't hoist allocation and dealloc stacks.
-  if (isa<AllocationInst>(Inst) || isa<DeallocStackInst>(Inst))
-    return false;
-
-  // Can't hoist instructions which may have side effects.
-  if (!hasNoSideEffect(Inst, SafeReads))
-    return false;
-
-  // The operands need to be loop invariant.
-  if (!hasLoopInvariantOperands(Inst, Loop)) {
-    DEBUG(llvm::dbgs() << "   loop variant operands\n");
-    return false;
-  }
-  
-  return true;
-}
-
-static bool hoistInstructions(SILLoop *Loop, DominanceInfo *DT,
-                              ReadSet &SafeReads, bool RunsOnHighLevelSil) {
-  auto Preheader = Loop->getLoopPreheader();
-  if (!Preheader)
-    return false;
-
-  DEBUG(llvm::dbgs() << " Hoisting instructions.\n");
-
   auto HeaderBB = Loop->getHeader();
-  bool Changed = false;
-
-  // Traverse the dominator tree starting at the loop header. Hoisting
-  // instructions as we go.
   auto DTRoot = DT->getNode(HeaderBB);
   SmallVector<SILBasicBlock *, 8> ExitingBBs;
   Loop->getExitingBlocks(ExitingBBs);
@@ -201,96 +137,53 @@ static bool hoistInstructions(SILLoop *Loop, DominanceInfo *DT,
       It.skipChildren();
       continue;
     }
+    domBlocks.push_back(CurBB);
+    // Next block in dominator tree.
+    ++It;
+  }
+}
 
+static bool hoistInstruction(DominanceInfo *DT, SILInstruction *Inst,
+                             SILLoop *Loop, SILBasicBlock *&Preheader) {
+  if (!hasLoopInvariantOperands(Inst, Loop)) {
+    DEBUG(llvm::dbgs() << "   loop variant operands\n");
+    return false;
+  }
+  auto mvBefore = Preheader->getTerminator();
+  ArraySemanticsCall semCall(Inst);
+  if (semCall.canHoist(mvBefore, DT)) {
+    semCall.hoist(mvBefore, DT);
+  } else {
+    Inst->moveBefore(mvBefore);
+  }
+  return true;
+}
+
+static bool hoistInstructions(SILLoop *Loop, DominanceInfo *DT,
+                              HoistSet &HoistUpSet) {
+  DEBUG(llvm::dbgs() << " Hoisting instructions.\n");
+  auto Preheader = Loop->getLoopPreheader();
+  assert(Preheader && "Expected a preheader");
+  bool Changed = false;
+  SmallVector<SILBasicBlock *, 8> domBlocks;
+  getDominatingBlocks(domBlocks, Loop, DT);
+
+  for (auto *CurBB : domBlocks) {
     // We now that the block is guaranteed to be executed. Hoist if we can.
     for (auto InstIt = CurBB->begin(), E = CurBB->end(); InstIt != E;) {
       SILInstruction *Inst = &*InstIt;
       ++InstIt;
       DEBUG(llvm::dbgs() << "  looking at " << *Inst);
-      if (canHoistInstruction(Inst, Loop, SafeReads)) {
+      if (!HoistUpSet.count(Inst)) {
-        DEBUG(llvm::dbgs() << "   hoisting to preheader.\n");
+        continue;
-        Changed = true;
-        Inst->moveBefore(Preheader->getTerminator());
-      } else if (RunsOnHighLevelSil) {
-        ArraySemanticsCall semCall(Inst);
-        switch (semCall.getKind()) {
-        case ArrayCallKind::kGetCount:
-        case ArrayCallKind::kGetCapacity:
-          if (hasLoopInvariantOperands(Inst, Loop) &&
-              semCall.canHoist(Preheader->getTerminator(), DT)) {
-            Changed = true;
-            semCall.hoist(Preheader->getTerminator(), DT);
       }
-          break;
+      if (!hoistInstruction(DT, Inst, Loop, Preheader)) {
-        default:
+        continue;
-          break;
       }
-      }
+      DEBUG(llvm::dbgs() << "Hoisted " << *Inst);
-    }
-
-    // Next block in dominator tree.
-    ++It;
-  }
-  return Changed;
-}
-
-static bool sinkFixLifetime(SILLoop *Loop, DominanceInfo *DomTree,
-                            SILLoopInfo *LI) {
-  DEBUG(llvm::errs() << " Sink fix_lifetime attempt\n");
-  auto Preheader = Loop->getLoopPreheader();
-  if (!Preheader)
-    return false;
-
-  // Only handle innermost loops for now.
-  if (!Loop->getSubLoops().empty())
-    return false;
-
-  // Only handle single exit blocks for now.
-  auto *ExitBB = Loop->getExitBlock();
-  if (!ExitBB)
-    return false;
-  auto *ExitingBB = Loop->getExitingBlock();
-  if (!ExitingBB)
-    return false;
-
-  // We can sink fix_lifetime instructions if there are no reference counting
-  // instructions in the loop.
-  SmallVector<FixLifetimeInst *, 16> FixLifetimeInsts;
-  for (auto *BB : Loop->getBlocks()) {
-    for (auto &Inst : *BB) {
-      if (auto FLI = dyn_cast<FixLifetimeInst>(&Inst)) {
-        FixLifetimeInsts.push_back(FLI);
-      } else if (Inst.mayHaveSideEffects() && !isa<LoadInst>(&Inst) &&
-                 !isa<StoreInst>(&Inst)) {
-        DEBUG(llvm::errs() << "  mayhavesideeffects because of" << Inst);
-        DEBUG(Inst.getParent()->dump());
-        return false;
-      }
-    }
-  }
-
-  // Sink the fix_lifetime instruction.
-  bool Changed = false;
-  for (auto *FLI : FixLifetimeInsts)
-    if (DomTree->dominates(FLI->getOperand()->getParentBlock(),
-                           Preheader)) {
-      auto Succs = ExitingBB->getSuccessors();
-      for (unsigned EdgeIdx = 0; EdgeIdx <  Succs.size(); ++EdgeIdx) {
-        SILBasicBlock *BB = Succs[EdgeIdx];
-        if (BB == ExitBB) {
-          auto *SplitBB = splitCriticalEdge(ExitingBB->getTerminator(), EdgeIdx,
-                                            DomTree, LI);
-          auto *OutsideBB = SplitBB ? SplitBB : ExitBB;
-          // Update the ExitBB.
-          ExitBB = OutsideBB;
-          DEBUG(llvm::errs() << "  moving fix_lifetime to exit BB " << *FLI);
-          FLI->moveBefore(&*OutsideBB->begin());
       Changed = true;
     }
   }
-    } else {
-      DEBUG(llvm::errs() << "  does not dominate " << *FLI);
-    }
 
   return Changed;
 }
@@ -306,7 +199,7 @@ struct LoopNestSummary {
 
 
   void copySummary(LoopNestSummary &Other) {
-    MayWrites.append(Other.MayWrites.begin(), Other.MayWrites.end());
+    MayWrites.insert(Other.MayWrites.begin(), Other.MayWrites.end());
   }
 
   LoopNestSummary(const LoopNestSummary &) = delete;
@@ -314,6 +207,115 @@ struct LoopNestSummary {
   LoopNestSummary(LoopNestSummary &&) = delete;
 };
 
+static bool sinkInstruction(DominanceInfo *DT,
+                            std::unique_ptr<LoopNestSummary> &LoopSummary,
+                            SILInstruction *Inst, SILLoopInfo *LI) {
+  auto *Loop = LoopSummary->Loop;
+  SmallVector<SILBasicBlock *, 8> ExitBBs;
+  Loop->getExitBlocks(ExitBBs);
+  SmallVector<SILBasicBlock *, 8> NewExitBBs;
+  SmallVector<SILBasicBlock *, 8> ExitingBBs;
+  Loop->getExitingBlocks(ExitingBBs);
+  auto *ExitBB = Loop->getExitBlock();
+
+  bool Changed = false;
+  for (auto *ExitingBB : ExitingBBs) {
+    auto Succs = ExitingBB->getSuccessors();
+    for (unsigned EdgeIdx = 0; EdgeIdx < Succs.size(); ++EdgeIdx) {
+      SILBasicBlock *BB = Succs[EdgeIdx];
+      if (std::find(NewExitBBs.begin(), NewExitBBs.end(), BB) !=
+          NewExitBBs.end()) {
+        // Already got a copy there
+        continue;
+      }
+      SILBasicBlock *OutsideBB = nullptr;
+      if (std::find(ExitBBs.begin(), ExitBBs.end(), BB) != ExitBBs.end()) {
+        auto *SplitBB =
+            splitCriticalEdge(ExitingBB->getTerminator(), EdgeIdx, DT, LI);
+        OutsideBB = SplitBB ? SplitBB : BB;
+        NewExitBBs.push_back(OutsideBB);
+      }
+      if (!OutsideBB) {
+        continue;
+      }
+      // If OutsideBB already contains Inst -> skip
+      // This might happen if we have a conditional control flow
+      // And a pair
+      // We hoisted the first part, we can safely ignore sinking
+      auto matchPred = [&](SILInstruction &CurrIns) {
+        return Inst->isIdenticalTo(&CurrIns);
+      };
+      if (std::find_if(OutsideBB->begin(), OutsideBB->end(), matchPred) !=
+          OutsideBB->end()) {
+        DEBUG(llvm::errs() << "  instruction already at exit BB " << *Inst);
+        ExitBB = nullptr;
+      } else if (ExitBB) {
+        // easy case
+        DEBUG(llvm::errs() << "  moving instruction to exit BB " << *Inst);
+        Inst->moveBefore(&*OutsideBB->begin());
+      } else {
+        DEBUG(llvm::errs() << "  cloning instruction to exit BB " << *Inst);
+        Inst->clone(&*OutsideBB->begin());
+      }
+      Changed = true;
+    }
+  }
+  if (Changed && !ExitBB) {
+    // Created clones of instruction
+    // Remove it from the may write set - dangling pointer
+    LoopSummary->MayWrites.erase(Inst);
+    Inst->getParent()->erase(Inst);
+  }
+  return Changed;
+}
+
+static bool sinkInstructions(std::unique_ptr<LoopNestSummary> &LoopSummary,
+                             DominanceInfo *DT, SILLoopInfo *LI,
+                             HoistSet &SinkDownSet) {
+  auto *Loop = LoopSummary->Loop;
+  DEBUG(llvm::errs() << " Sink instructions attempt\n");
+  SmallVector<SILBasicBlock *, 8> domBlocks;
+  getDominatingBlocks(domBlocks, Loop, DT);
+
+  bool Changed = false;
+  for (auto *Inst : SinkDownSet) {
+    // only sink if the block is guaranteed to be executed.
+    if (std::find(domBlocks.begin(), domBlocks.end(), Inst->getParent()) ==
+        domBlocks.end()) {
+      continue;
+    }
+    Changed |= sinkInstruction(DT, LoopSummary, Inst, LI);
+  }
+
+  return Changed;
+}
+
+static bool
+hoistAndSinkInstructionPairs(std::unique_ptr<LoopNestSummary> &LoopSummary,
+                             DominanceInfo *DT, SILLoopInfo *LI,
+                             HoistPairSet &Pairs) {
+  auto *Loop = LoopSummary->Loop;
+  DEBUG(llvm::errs() << " Hoist and Sink pairs attempt\n");
+  auto Preheader = Loop->getLoopPreheader();
+  assert(Preheader && "Expected a preheader");
+
+  bool Changed = false;
+  for (auto pair : Pairs) {
+    auto *Up = pair.first;
+    auto *Down = pair.second;
+    if (!hoistInstruction(DT, Up, Loop, Preheader)) {
+      continue;
+    }
+    DEBUG(llvm::dbgs() << "Hoisted " << *Up);
+    if (!sinkInstruction(DT, LoopSummary, Down, LI)) {
+      llvm_unreachable("LICM: Could not perform must-sink instruction");
+    }
+    DEBUG(llvm::errs() << " Successfully hosited and sank pair\n");
+    Changed = true;
+  }
+  return Changed;
+}
+
 /// \brief Optimize the loop tree bottom up propagating loop's summaries up the
 /// loop tree.
 class LoopTreeOptimization {
@@ -328,15 +330,23 @@ class LoopTreeOptimization {
 
   /// True if LICM is done on high-level SIL, i.e. semantic calls are not
   /// inlined yet. In this case some semantic calls can be hoisted.
-  bool RunsOnHighLevelSil;
+  bool RunsOnHighLevelSIL;
+
+  /// Instructions that we may be able to hoist up
+  HoistSet HoistUp;
+
+  /// Instructions that we may be able to sink down
+  HoistSet SinkDown;
+
+  /// Instruction pairs that we may be able to hoist and sink
+  HoistPairSet HoistingPairs;
 
 public:
   LoopTreeOptimization(SILLoop *TopLevelLoop, SILLoopInfo *LI,
                        AliasAnalysis *AA, SideEffectAnalysis *SEA,
-                       DominanceInfo *DT,
+                       DominanceInfo *DT, bool RunsOnHighLevelSil)
-                       bool RunsOnHighLevelSil)
       : LoopInfo(LI), AA(AA), SEA(SEA), DomTree(DT), Changed(false),
-        RunsOnHighLevelSil(RunsOnHighLevelSil) {
+        RunsOnHighLevelSIL(RunsOnHighLevelSil) {
     // Collect loops for a recursive bottom-up traversal in the loop tree.
     BotUpWorkList.push_back(TopLevelLoop);
     for (unsigned i = 0; i < BotUpWorkList.size(); ++i) {
@@ -353,12 +363,11 @@ protected:
   /// \brief Propagate the sub-loops' summaries up to the current loop.
   void propagateSummaries(std::unique_ptr<LoopNestSummary> &CurrSummary);
 
-  /// \brief Collect a set of reads that can be hoisted to the loop's preheader.
+  /// \brief Collect a set of instructions that can be hoisted
-  void analyzeCurrentLoop(std::unique_ptr<LoopNestSummary> &CurrSummary,
+  void analyzeCurrentLoop(std::unique_ptr<LoopNestSummary> &CurrSummary);
-                          ReadSet &SafeReads);
 
   /// \brief Optimize the current loop nest.
-  void optimizeLoop(SILLoop *CurrentLoop, ReadSet &SafeReads);
+  bool optimizeLoop(std::unique_ptr<LoopNestSummary> &CurrSummary);
 };
 } // end anonymous namespace
 
@@ -374,11 +383,23 @@ bool LoopTreeOptimization::optimize() {
     auto CurrLoopSummary = llvm::make_unique<LoopNestSummary>(CurrentLoop);
     propagateSummaries(CurrLoopSummary);
 
-    // Analyze the current loop for reads that can be hoisted.
+    // If the current loop changed, then we might reveal more instr to hoist
-    ReadSet SafeReads;
+    // For example, a fix_lifetime's operand, if hoisted outside,
-    analyzeCurrentLoop(CurrLoopSummary, SafeReads);
+    // Might allow us to sink the instruction out of the loop
+    bool currChanged = false;
+    do {
+      currChanged = false;
 
-    optimizeLoop(CurrentLoop, SafeReads);
+      // Analyze the current loop for instructions that can be hoisted.
+      analyzeCurrentLoop(CurrLoopSummary);
+
+      currChanged = optimizeLoop(CurrLoopSummary);
+
+      // Reset the data structures for next loop in the list
+      HoistUp.clear();
+      SinkDown.clear();
+      HoistingPairs.clear();
+    } while (currChanged);
 
     // Store the summary for parent loops to use.
     LoopNestSummaryMap[CurrentLoop] = std::move(CurrLoopSummary);
@@ -395,56 +416,225 @@ void LoopTreeOptimization::propagateSummaries(
   }
 }
 
+static bool isSafeReadOnlyApply(SideEffectAnalysis *SEA, ApplyInst *AI) {
+  FunctionSideEffects E;
+  SEA->getCalleeEffects(E, AI);
+
+  if (E.getGlobalEffects().mayRead()) {
+    // If we have Global effects,
+    // we don't know which memory is read in the callee.
+    // Therefore we bail for safety
+    return false;
+  }
+
+  auto MB = E.getMemBehavior(RetainObserveKind::ObserveRetains);
+  return (MB <= SILInstruction::MemoryBehavior::MayRead);
+}
+
+static void checkSideEffects(swift::SILInstruction &Inst, WriteSet &MayWrites) {
+  if (Inst.mayHaveSideEffects()) {
+    MayWrites.insert(&Inst);
+  }
+}
+
+/// Returns true if the \p Inst follows the default hoisting heuristic
+static bool canHoistUpDefault(SILInstruction *inst, SILLoop *Loop,
+                              DominanceInfo *DT, bool RunsOnHighLevelSil) {
+  auto Preheader = Loop->getLoopPreheader();
+  if (!Preheader) {
+    return false;
+  }
+
+  if (isa<TermInst>(inst) || isa<AllocationInst>(inst) ||
+      isa<DeallocationInst>(inst)) {
+    return false;
+  }
+
+  if (inst->getMemoryBehavior() == SILInstruction::MemoryBehavior::None) {
+    return true;
+  }
+
+  if (!RunsOnHighLevelSil) {
+    return false;
+  }
+
+  ArraySemanticsCall semCall(inst);
+  return semCall.canHoist(Preheader->getTerminator(), DT);
+}
+
+static void analyzeBeginAccess(BeginAccessInst *&BI,
+                               SmallVector<BeginAccessInst *, 8> &BeginAccesses,
+                               SmallVector<EndAccessInst *, 8> &EndAccesses,
+                               HoistPairSet &HoistingPairs) {
+  if (BI->getEnforcement() != SILAccessEnforcement::Dynamic) {
+    return;
+  }
+
+  const AccessedStorage &storage =
+      findAccessedStorageNonNested(BI->getSource());
+  if (!storage) {
+    return;
+  }
+
+  // find matching end access:
+  auto matchingEndPred = [&](EndAccessInst *EI) {
+    return EI->getBeginAccess() == BI;
+  };
+  auto matchingEnd =
+      std::find_if(EndAccesses.begin(), EndAccesses.end(), matchingEndPred);
+  if (matchingEnd == EndAccesses.end()) {
+    // no matching end within the loop
+    return;
+  }
+  auto *EI = *matchingEnd;
+  ++matchingEnd;
+  if (std::find_if(matchingEnd, EndAccesses.end(), matchingEndPred) !=
+      EndAccesses.end()) {
+    // We expect a single matching end access in the loop
+    return;
+  }
+
+  auto BIAccessedStorageNonNested = findAccessedStorageNonNested(BI);
+  auto safeBeginPred = [&](BeginAccessInst *OtherBI) {
+    if (BI == OtherBI) {
+      return true;
+    }
+    return BIAccessedStorageNonNested.isDistinctFrom(
+        findAccessedStorageNonNested(OtherBI));
+  };
+
+  if (std::all_of(BeginAccesses.begin(), BeginAccesses.end(), safeBeginPred)) {
+    HoistingPairs.push_back(std::make_pair(BI, EI));
+  }
+}
+
+// Analyzes current loop for hosting/sinking potential:
+// Computes set of instructions we may be able to move out of the loop
+// Important Note:
+// We can't bail out of this method! we have to run it on all loops.
+// We *need* to discover all MayWrites -
+// even if the loop is otherwise skipped!
+// This is because outer loops will depend on the inner loop's writes.
 void LoopTreeOptimization::analyzeCurrentLoop(
-    std::unique_ptr<LoopNestSummary> &CurrSummary, ReadSet &SafeReads) {
+    std::unique_ptr<LoopNestSummary> &CurrSummary) {
   WriteSet &MayWrites = CurrSummary->MayWrites;
   SILLoop *Loop = CurrSummary->Loop;
   DEBUG(llvm::dbgs() << " Analyzing accesses.\n");
 
   // Contains function calls in the loop, which only read from memory.
   SmallVector<ApplyInst *, 8> ReadOnlyApplies;
+  // Contains Loads inside the loop.
+  SmallVector<LoadInst *, 8> Loads;
+  // Contains fix_lifetime, we might be able to sink them.
+  SmallVector<FixLifetimeInst *, 8> FixLifetimes;
+  // Contains begin_access, we might be able to hoist them.
+  SmallVector<BeginAccessInst *, 8> BeginAccesses;
+  // Contains end_access, we might be able to sink them.
+  SmallVector<EndAccessInst *, 8> EndAccesses;
 
   for (auto *BB : Loop->getBlocks()) {
     for (auto &Inst : *BB) {
-      // Ignore fix_lifetime instructions.
+      switch (Inst.getKind()) {
-      if (isa<FixLifetimeInst>(&Inst))
+      case SILInstructionKind::FixLifetimeInst: {
-        continue;
+        auto *FL = dyn_cast<FixLifetimeInst>(&Inst);
-
+        assert(FL && "Expected a FixLifetime instruction");
-      // Collect loads.
+        FixLifetimes.push_back(FL);
-      auto LI = dyn_cast<LoadInst>(&Inst);
+        // We can ignore the side effects of FixLifetimes
-      if (LI) {
+        break;
-        if (!mayWriteTo(AA, MayWrites, LI))
-          SafeReads.insert(LI);
-        continue;
       }
-      if (auto *AI = dyn_cast<ApplyInst>(&Inst)) {
+      case SILInstructionKind::LoadInst: {
-        // In contrast to load instructions, we first collect all read-only
+        auto *LI = dyn_cast<LoadInst>(&Inst);
-        // function calls and add them later to SafeReads.
+        assert(LI && "Expected a Load instruction");
-        FunctionSideEffects E;
+        Loads.push_back(LI);
-        SEA->getCalleeEffects(E, AI);
+        break;
-
+      }
-        auto MB = E.getMemBehavior(RetainObserveKind::ObserveRetains);
+      case SILInstructionKind::BeginAccessInst: {
-        if (MB <= SILInstruction::MemoryBehavior::MayRead)
+        auto *BI = dyn_cast<BeginAccessInst>(&Inst);
+        assert(BI && "Expected a Begin Access");
+        BeginAccesses.push_back(BI);
+        checkSideEffects(Inst, MayWrites);
+        break;
+      }
+      case SILInstructionKind::EndAccessInst: {
+        auto *EI = dyn_cast<EndAccessInst>(&Inst);
+        assert(EI && "Expected an End Access");
+        EndAccesses.push_back(EI);
+        checkSideEffects(Inst, MayWrites);
+        break;
+      }
+      case swift::SILInstructionKind::CondFailInst: {
+        // We can (and must) hoist cond_fail instructions if the operand is
+        // invariant. We must hoist them so that we preserve memory safety. A
+        // cond_fail that would have protected (executed before) a memory access
+        // must - after hoisting - also be executed before said access.
+        HoistUp.insert(&Inst);
+        checkSideEffects(Inst, MayWrites);
+        break;
+      }
+      case SILInstructionKind::ApplyInst: {
+        auto *AI = dyn_cast<ApplyInst>(&Inst);
+        assert(AI && "Expected an Apply Instruction");
+        if (isSafeReadOnlyApply(SEA, AI)) {
           ReadOnlyApplies.push_back(AI);
         }
-      if (Inst.mayHaveSideEffects()) {
+        // check for array semantics and side effects - same as default
-        MayWrites.push_back(&Inst);
+        LLVM_FALLTHROUGH;
-        // Remove clobbered loads we have seen before.
+      }
-        removeWrittenTo(AA, SafeReads, &Inst);
+      default: {
+        checkSideEffects(Inst, MayWrites);
+        if (canHoistUpDefault(&Inst, Loop, DomTree, RunsOnHighLevelSIL)) {
+          HoistUp.insert(&Inst);
+        }
+        break;
       }
       }
     }
-  for (auto *AI : ReadOnlyApplies) {
-    if (!mayWriteTo(AA, SEA, MayWrites, AI))
-      SafeReads.insert(AI);
-  }
   }
 
-void LoopTreeOptimization::optimizeLoop(SILLoop *CurrentLoop,
+  auto *Preheader = Loop->getLoopPreheader();
-                                        ReadSet &SafeReads) {
+  if (!Preheader) {
-  Changed |= hoistInstructions(CurrentLoop, DomTree, SafeReads,
+    // Can't hoist/sink instructions
-                               RunsOnHighLevelSil);
+    return;
-  Changed |= sinkFixLifetime(CurrentLoop, DomTree, LoopInfo);
+  }
+  for (auto *AI : ReadOnlyApplies) {
+    if (!mayWriteTo(AA, SEA, MayWrites, AI)) {
+      HoistUp.insert(AI);
+    }
+  }
+  for (auto *LI : Loads) {
+    if (!mayWriteTo(AA, MayWrites, LI)) {
+      HoistUp.insert(LI);
+    }
+  }
+  bool mayWritesMayRelease =
+      std::any_of(MayWrites.begin(), MayWrites.end(),
+                  [&](SILInstruction *W) { return W->mayRelease(); });
+  for (auto *FL : FixLifetimes) {
+    if (!DomTree->dominates(FL->getOperand()->getParentBlock(), Preheader)) {
+      continue;
+    }
+    if (!mayWriteTo(AA, MayWrites, FL) || !mayWritesMayRelease) {
+      SinkDown.insert(FL);
+    }
+  }
+  for (auto *BI : BeginAccesses) {
+    analyzeBeginAccess(BI, BeginAccesses, EndAccesses, HoistingPairs);
+  }
+}
+
+bool LoopTreeOptimization::optimizeLoop(
+    std::unique_ptr<LoopNestSummary> &CurrSummary) {
+  auto *CurrentLoop = CurrSummary->Loop;
+  // We only support Loops with a preheader
+  if (!CurrentLoop->getLoopPreheader())
+    return false;
+  bool currChanged = false;
+  currChanged |= hoistInstructions(CurrentLoop, DomTree, HoistUp);
+  currChanged |= sinkInstructions(CurrSummary, DomTree, LoopInfo, SinkDown);
+  currChanged |= hoistAndSinkInstructionPairs(CurrSummary, DomTree, LoopInfo,
+                                              HoistingPairs);
+  Changed |= currChanged;
+  return currChanged;
 }
 
 namespace {

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -193,7 +193,12 @@ void addHighLevelLoopOptPasses(SILPassPipelinePlan &P) {
   P.addHighLevelCSE();
   P.addSILCombine();
   P.addSimplifyCFG();
+  // Optimize access markers for better LICM: might merge accesses
+  // It will also set the no_nested_conflict for dynamic accesses
+  P.addAccessEnforcementOpts();
   P.addHighLevelLICM();
+  // Simplify CFG after LICM that creates new exit blocks
+  P.addSimplifyCFG();
   // Start of loop unrolling passes.
   P.addArrayCountPropagation();
   // To simplify induction variable.
@@ -446,7 +451,12 @@ static void addLateLoopOptPassPipeline(SILPassPipelinePlan &P) {
 
   // Perform the final lowering transformations.
   P.addCodeSinking();
+  // Optimize access markers for better LICM: might merge accesses
+  // It will also set the no_nested_conflict for dynamic accesses
+  P.addAccessEnforcementOpts();
   P.addLICM();
+  // Simplify CFG after LICM that creates new exit blocks
+  P.addSimplifyCFG();
 
   // Optimize overflow checks.
   P.addRedundantOverflowCheckRemoval();

test/SILOptimizer/array_mutable_assertonly.swift

@@ -87,7 +87,6 @@ struct Array2d {
 // TEST6-LABEL: COW Array Opts in Func {{.*}}test2DArrayLoop{{.*}}
 // TEST6:        Array Opts in Loop Loop at depth 2
 // TEST6-NOT:   COW Array Opts in
-// TEST6:        Hoisting make_mutable
 // TEST6:        Array Opts in Loop Loop at depth 1
 // TEST6-NOT:   COW Array Opts in
 // TEST6:        Hoisting make_mutable

test/SILOptimizer/licm_exclusivity.swift

@@ -0,0 +1,50 @@
+// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -Xllvm -debug-only=sil-licm -primary-file %s 2>&1 | %FileCheck %s --check-prefix=TEST1
+// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -Xllvm -debug-only=sil-licm  -primary-file %s 2>&1 | %FileCheck %s --check-prefix=TEST2
+// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil  -primary-file %s | %FileCheck %s --check-prefix=TESTSIL
+// REQUIRES: optimized_stdlib
+
+// TEST1-LABEL: Processing loops in {{.*}}run_ReversedArray{{.*}}
+// TEST1: Hoist and Sink pairs attempt
+// TEST1: Hoisted
+// TEST1: Successfully hosited and sank pair
+
+// TESTSIL-LABEL: sil hidden @$S16licm_exclusivity17run_ReversedArrayyySiF : $@convention(thin) (Int) -> () {
+// TESTSIL: bb
+// TESTSIL: begin_access [modify] [dynamic] [no_nested_conflict]
+// TESTSIL: br bb{{.*}}
+// TESTSIL-NEXT bb{{.*}}:
+// TESTSIL: end_access
+// TESTSIL: return
+var x = 0
+func run_ReversedArray(_ N: Int) {
+  let array = Array(repeating: 1, count: 42)
+  let reversedArray = array.reversed()
+
+  // Iterate over the underlying type
+  // ReversedRandomAccessCollection<Array<Int>>
+  for _ in 1...N {
+    for item in reversedArray {
+      x = item
+    }
+  }
+}
+
+// TEST2-LABEL: Processing loops in {{.*}}count_unicodeScalars{{.*}}
+// TEST2: Hoist and Sink pairs attempt
+// TEST2: Hoisted
+// TEST2: cloning
+// TEST2: Successfully hosited and sank pair
+
+// TESTSIL-LABEL: sil @$S16licm_exclusivity20count_unicodeScalarsyySS17UnicodeScalarViewVF : $@convention(thin) (@guaranteed String.UnicodeScalarView) -> () {
+// TESTSIL: bb0(%0 : $String.UnicodeScalarView)
+// TESTSIL-NEXT: %1 = global_addr @$S16licm_exclusivity5countSivp : $*Int
+// TESTSIL: begin_access [modify] [dynamic] [no_nested_conflict] %1 : $*Int
+// TESTSIL-NEXT: br bb1
+// TESTSIL: end_access
+// TESTSIL: return
+var count: Int = 0
+public func count_unicodeScalars(_ s: String.UnicodeScalarView) {
+  for _ in s {
+    count += 1
+  }
+}