Split predictable mem opts into two different passes, one that runs before diagnostics and one that runs after diagnostics.

I discovered while updating PMO for ownership that for ~5 years there has been a
bug where we were treating copy_addr of trivial values like an "Assign" (in PMO
terminology) of a non-trivial value and thus stopping allocation
elimination. When I fixed this I discovered that this caused us to no longer
emit diagnostics in a predictable way. Specifically, consider the following
swift snippet:

  var _: UInt = (-1) >> 0

Today, we emit a diagnostic that -1 can not be put into a UInt. This occurs
since even though the underlying allocation is only stored into, the copy_addr
assign keeps it alive, causing the diagnostics pass to see the conversion. With
my fix though, we see that we are only storing into the allocation, causing the
allocation to be eliminated before the constant propagation diagnostic pass
runs, causing the diagnostic to no longer be emitted.

We should truly not be performing this type of DCE before we emit such
diagnostics. So in this commit, I split the pass into two parts:

1. A load promotion pass that performs the SSA formation needed for SSA based
diagnostics to actually work.

2. An allocation elimination passes that run /after/ SSA based diagnostics.

This should be NFC since the constant propagation SSA based diagnostics do not
create memory operations so the output should be the same.

Author: gottesmm

include/swift/SILOptimizer/PassManager/Passes.def

@@ -232,8 +232,10 @@ PASS(PerfInliner, "inline",
      "Performance Function Inlining")
 PASS(PerformanceConstantPropagation, "performance-constant-propagation",
      "Constant Propagation for Performance without Diagnostics")
-PASS(PredictableMemoryOptimizations, "predictable-memopt",
-     "Predictable Memory Optimization for Diagnostics")
+PASS(PredictableMemoryAccessOptimizations, "predictable-memaccess-opts",
+     "Predictable Memory Access Optimizations for Diagnostics")
+PASS(PredictableDeadAllocationElimination, "predictable-deadalloc-elim",
+     "Eliminate dead temporary allocations after diagnostics")
 PASS(ReleaseDevirtualizer, "release-devirtualizer",
      "SIL release Devirtualization")
 PASS(RetainSinking, "retain-sinking",

lib/SILOptimizer/Mandatory/PredictableMemOpt.cpp

@@ -1011,16 +1011,15 @@ class AllocOptimize {
   AllocOptimize(AllocationInst *TheMemory, SmallVectorImpl<PMOMemoryUse> &Uses,
                 SmallVectorImpl<SILInstruction *> &Releases);
 
-  bool doIt();
+  bool optimizeMemoryAccesses();
+  bool tryToRemoveDeadAllocation();
 
 private:
   bool promoteLoad(SILInstruction *Inst);
   void promoteDestroyAddr(DestroyAddrInst *DAI,
                           MutableArrayRef<AvailableValue> Values);
   bool canPromoteDestroyAddr(DestroyAddrInst *DAI,
                              SmallVectorImpl<AvailableValue> &AvailableValues);
-
-  bool tryToRemoveDeadAllocation();
 };
 
 } // end anonymous namespace
@@ -1376,79 +1375,135 @@ bool AllocOptimize::tryToRemoveDeadAllocation() {
   return true;
 }
 
-/// doIt - returns true on error.
-bool AllocOptimize::doIt() {
-  bool Changed = false;
-
-  // Don't try to optimize incomplete aggregates.
-  if (MemoryType.aggregateHasUnreferenceableStorage())
-    return false;
+bool AllocOptimize::optimizeMemoryAccesses() {
+  bool changed = false;
 
   // If we've successfully checked all of the definitive initialization
   // requirements, try to promote loads.  This can explode copy_addrs, so the
   // use list may change size.
   for (unsigned i = 0; i != Uses.size(); ++i) {
-    auto &Use = Uses[i];
+    auto &use = Uses[i];
     // Ignore entries for instructions that got expanded along the way.
-    if (Use.Inst && Use.Kind == PMOUseKind::Load) {
-      if (promoteLoad(Use.Inst)) {
+    if (use.Inst && use.Kind == PMOUseKind::Load) {
+      if (promoteLoad(use.Inst)) {
         Uses[i].Inst = nullptr;  // remove entry if load got deleted.
-        Changed = true;
+        changed = true;
       }
     }
   }
-  
-  // If this is an allocation, try to remove it completely.
-  Changed |= tryToRemoveDeadAllocation();
 
-  return Changed;
+  return changed;
+}
+
+//===----------------------------------------------------------------------===//
+//                           Top Level Entrypoints
+//===----------------------------------------------------------------------===//
+
+static AllocationInst *getOptimizableAllocation(SILInstruction *i) {
+  if (!isa<AllocBoxInst>(i) && !isa<AllocStackInst>(i)) {
+    return nullptr;
+  }
+
+  auto *alloc = cast<AllocationInst>(i);
+
+  // If our aggregate has unreferencable storage, we can't optimize. Return
+  // nullptr.
+  if (getMemoryType(alloc).aggregateHasUnreferenceableStorage())
+    return nullptr;
+
+  // Otherwise we are good to go. Lets try to optimize this memory!
+  return alloc;
 }
 
-static bool optimizeMemoryAllocations(SILFunction &Fn) {
-  bool Changed = false;
-  for (auto &BB : Fn) {
-    auto I = BB.begin(), E = BB.end();
-    while (I != E) {
-      SILInstruction *Inst = &*I;
-      if (!isa<AllocBoxInst>(Inst) && !isa<AllocStackInst>(Inst)) {
-        ++I;
+static bool optimizeMemoryAccesses(SILFunction &fn) {
+  bool changed = false;
+  for (auto &bb : fn) {
+    auto i = bb.begin(), e = bb.end();
+    while (i != e) {
+      // First see if i is an allocation that we can optimize. If not, skip it.
+      AllocationInst *alloc = getOptimizableAllocation(&*i);
+      if (!alloc) {
+        ++i;
         continue;
       }
-      auto Alloc = cast<AllocationInst>(Inst);
 
-      LLVM_DEBUG(llvm::dbgs() << "*** PMO Optimize looking at: " << *Alloc
-                              << "\n");
-      PMOMemoryObjectInfo MemInfo(Alloc);
+      LLVM_DEBUG(llvm::dbgs() << "*** PMO Optimize Memory Accesses looking at: "
+                              << *alloc << "\n");
+      PMOMemoryObjectInfo memInfo(alloc);
 
       // Set up the datastructure used to collect the uses of the allocation.
-      SmallVector<PMOMemoryUse, 16> Uses;
-      SmallVector<SILInstruction*, 4> Releases;
+      SmallVector<PMOMemoryUse, 16> uses;
+      SmallVector<SILInstruction *, 4> destroys;
 
       // Walk the use list of the pointer, collecting them. If we are not able
       // to optimize, skip this value. *NOTE* We may still scalarize values
       // inside the value.
-      if (!collectPMOElementUsesFrom(MemInfo, Uses, Releases)) {
-        ++I;
+      if (!collectPMOElementUsesFrom(memInfo, uses, destroys)) {
+        ++i;
         continue;
       }
 
-      Changed |= AllocOptimize(Alloc, Uses, Releases).doIt();
-      
-      // Carefully move iterator to avoid invalidation problems.
-      ++I;
-      if (Alloc->use_empty()) {
-        Alloc->eraseFromParent();
+      AllocOptimize allocOptimize(alloc, uses, destroys);
+      changed |= allocOptimize.optimizeMemoryAccesses();
+
+      // Move onto the next instruction. We know this is safe since we do not
+      // eliminate allocations here.
+      ++i;
+    }
+  }
+
+  return changed;
+}
+
+static bool eliminateDeadAllocations(SILFunction &fn) {
+  bool changed = false;
+  for (auto &bb : fn) {
+    auto i = bb.begin(), e = bb.end();
+    while (i != e) {
+      // First see if i is an allocation that we can optimize. If not, skip it.
+      AllocationInst *alloc = getOptimizableAllocation(&*i);
+      if (!alloc) {
+        ++i;
+        continue;
+      }
+
+      LLVM_DEBUG(llvm::dbgs()
+                 << "*** PMO Dead Allocation Elimination looking at: " << *alloc
+                 << "\n");
+      PMOMemoryObjectInfo memInfo(alloc);
+
+      // Set up the datastructure used to collect the uses of the allocation.
+      SmallVector<PMOMemoryUse, 16> uses;
+      SmallVector<SILInstruction *, 4> destroys;
+
+      // Walk the use list of the pointer, collecting them. If we are not able
+      // to optimize, skip this value. *NOTE* We may still scalarize values
+      // inside the value.
+      if (!collectPMOElementUsesFrom(memInfo, uses, destroys)) {
+        ++i;
+        continue;
+      }
+
+      AllocOptimize allocOptimize(alloc, uses, destroys);
+      changed |= allocOptimize.tryToRemoveDeadAllocation();
+
+      // Move onto the next instruction. We know this is safe since we do not
+      // eliminate allocations here.
+      ++i;
+      if (alloc->use_empty()) {
+        alloc->eraseFromParent();
         ++NumAllocRemoved;
-        Changed = true;
+        changed = true;
       }
     }
   }
-  return Changed;
+
+  return changed;
 }
 
 namespace {
 
-class PredictableMemoryOptimizations : public SILFunctionTransform {
+class PredictableMemoryAccessOptimizations : public SILFunctionTransform {
   /// The entry point to the transformation.
   ///
   /// FIXME: This pass should not need to rerun on deserialized
@@ -1457,14 +1512,25 @@ class PredictableMemoryOptimizations : public SILFunctionTransform {
   /// either indicates that this pass missing some opportunities the first time,
   /// or has a pass order dependency on other early passes.
   void run() override {
-    if (optimizeMemoryAllocations(*getFunction()))
+    // TODO: Can we invalidate here just instructions?
+    if (optimizeMemoryAccesses(*getFunction()))
+      invalidateAnalysis(SILAnalysis::InvalidationKind::FunctionBody);
+  }
+};
+
+class PredictableDeadAllocationElimination : public SILFunctionTransform {
+  void run() override {
+    if (eliminateDeadAllocations(*getFunction()))
       invalidateAnalysis(SILAnalysis::InvalidationKind::FunctionBody);
   }
 };
 
 } // end anonymous namespace
 
+SILTransform *swift::createPredictableMemoryAccessOptimizations() {
+  return new PredictableMemoryAccessOptimizations();
+}
 
-SILTransform *swift::createPredictableMemoryOptimizations() {
-  return new PredictableMemoryOptimizations();
+SILTransform *swift::createPredictableDeadAllocationElimination() {
+  return new PredictableDeadAllocationElimination();
 }

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -106,12 +106,20 @@ static void addMandatoryOptPipeline(SILPassPipelinePlan &P,
   P.addOwnershipModelEliminator();
   P.addMandatoryInlining();
   P.addMandatorySILLinker();
-  P.addPredictableMemoryOptimizations();
+
+  // Promote loads as necessary to ensure we have enough SSA formation to emit
+  // SSA based diagnostics.
+  P.addPredictableMemoryAccessOptimizations();
 
   // Diagnostic ConstantPropagation must be rerun on deserialized functions
   // because it is sensitive to the assert configuration.
   // Consequently, certain optimization passes beyond this point will also rerun.
   P.addDiagnosticConstantPropagation();
+
+  // Now that we have emitted constant propagation diagnostics, try to eliminate
+  // dead allocations.
+  P.addPredictableDeadAllocationElimination();
+
   P.addGuaranteedARCOpts();
   P.addDiagnoseUnreachable();
   P.addDiagnoseInfiniteRecursion();

test/SILOptimizer/predictable_memopt.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -enable-sil-verify-all %s -predictable-memopt  | %FileCheck %s
+// RUN: %target-sil-opt -enable-sil-verify-all %s -predictable-memaccess-opts -predictable-deadalloc-elim  | %FileCheck %s
 
 import Builtin
 import Swift