Merge pull request #4384 from apple/revert-4358-pass-manager

Revert "SILPassManager: After a new function is pushed on the stack don't res…"

Author: tkremenek

include/swift/SILOptimizer/PassManager/PassManager.h

@@ -42,21 +42,8 @@ class SILPassManager {
   /// A list of registered analysis.
   llvm::SmallVector<SILAnalysis *, 16> Analysis;
 
-  /// An entry in the FunctionWorkList.
-  struct WorklistEntry {
-    WorklistEntry(SILFunction *F) : F(F) { }
-
-    SILFunction *F;
-
-    /// The current position in the transform-list.
-    unsigned PipelineIdx = 0;
-
-    /// How many times the pipeline was restarted for the function.
-    unsigned NumRestarts = 0;
-  };
-
   /// The worklist of functions to be processed by function passes.
-  std::vector<WorklistEntry> FunctionWorklist;
+  std::vector<SILFunction *> FunctionWorklist;
 
   // Name of the current optimization stage for diagnostics.
   std::string StageName;
@@ -224,8 +211,9 @@ class SILPassManager {
   /// the module.
   void runModulePass(SILModuleTransform *SMT);
 
-  /// Run the pass \p SFT on the function \p F.
-  void runPassOnFunction(SILFunctionTransform *SFT, SILFunction *F);
+  /// Run the passes in \p FuncTransforms on the function \p F.
+  void runPassesOnFunction(PassList FuncTransforms, SILFunction *F,
+                           bool runToCompletion);
 
   /// Run the passes in \p FuncTransforms. Return true
   /// if the pass manager requested to stop the execution

lib/SILOptimizer/PassManager/PassManager.cpp

@@ -49,6 +49,10 @@ llvm::cl::opt<unsigned> SILNumOptPassesToRun(
     "sil-opt-pass-count", llvm::cl::init(UINT_MAX),
     llvm::cl::desc("Stop optimizing after <N> optimization passes"));
 
+llvm::cl::opt<unsigned> SILFunctionPassPipelineLimit("sil-pipeline-limit",
+                                                     llvm::cl::init(10),
+                                                     llvm::cl::desc(""));
+
 llvm::cl::opt<std::string> SILBreakOnFun(
     "sil-break-on-function", llvm::cl::init(""),
     llvm::cl::desc(
@@ -270,92 +274,110 @@ static bool breakBeforeRunning(StringRef fnName, StringRef passName) {
   return fnName == SILBreakOnFun && passName == SILBreakOnPass;
 }
 
-void SILPassManager::runPassOnFunction(SILFunctionTransform *SFT,
-                                       SILFunction *F) {
+void SILPassManager::runPassesOnFunction(PassList FuncTransforms,
+                                         SILFunction *F,
+                                         bool runToCompletion) {
+
+  const SILOptions &Options = getOptions();
+
+  CompletedPasses &completedPasses = CompletedPassesMap[F];
 
   assert(analysesUnlocked() && "Expected all analyses to be unlocked!");
 
-  PrettyStackTraceSILFunctionTransform X(SFT, NumPassesRun);
-  DebugPrintEnabler DebugPrint(NumPassesRun);
+  for (auto SFT : FuncTransforms) {
+    PrettyStackTraceSILFunctionTransform X(SFT, NumPassesRun);
+    DebugPrintEnabler DebugPrint(NumPassesRun);
+
+    SFT->injectPassManager(this);
+    SFT->injectFunction(F);
+
+    // If nothing changed since the last run of this pass, we can skip this
+    // pass.
+    if (completedPasses.test((size_t)SFT->getPassKind()) &&
+        !SILDisableSkippingPasses) {
+      if (SILPrintPassName)
+        llvm::dbgs() << "(Skip) Stage: " << StageName
+                     << " Pass: " << SFT->getName()
+                     << ", Function: " << F->getName() << "\n";
+      continue;
+    }
 
-  SFT->injectPassManager(this);
-  SFT->injectFunction(F);
+    if (isDisabled(SFT)) {
+      if (SILPrintPassName)
+        llvm::dbgs() << "(Disabled) Stage: " << StageName
+                     << " Pass: " << SFT->getName()
+                     << ", Function: " << F->getName() << "\n";
+      continue;
+    }
 
-  // If nothing changed since the last run of this pass, we can skip this
-  // pass.
-  CompletedPasses &completedPasses = CompletedPassesMap[F];
-  if (completedPasses.test((size_t)SFT->getPassKind()) &&
-      !SILDisableSkippingPasses) {
-    if (SILPrintPassName)
-      llvm::dbgs() << "  (Skip) Stage: " << StageName
-                   << " Pass: " << SFT->getName()
-                   << ", Function: " << F->getName() << "\n";
-    return;
-  }
+    CurrentPassHasInvalidated = false;
 
-  if (isDisabled(SFT)) {
     if (SILPrintPassName)
-      llvm::dbgs() << "  (Disabled) Stage: " << StageName
+      llvm::dbgs() << "#" << NumPassesRun << " Stage: " << StageName
                    << " Pass: " << SFT->getName()
                    << ", Function: " << F->getName() << "\n";
-    return;
-  }
 
-  CurrentPassHasInvalidated = false;
+    if (doPrintBefore(SFT, F)) {
+      llvm::dbgs() << "*** SIL function before " << StageName << " "
+                   << SFT->getName() << " (" << NumOptimizationIterations
+                   << ") ***\n";
+      F->dump(Options.EmitVerboseSIL);
+    }
 
-  if (SILPrintPassName)
-    llvm::dbgs() << "  #" << NumPassesRun << " Stage: " << StageName
-                 << " Pass: " << SFT->getName()
-                 << ", Function: " << F->getName() << "\n";
+    llvm::sys::TimeValue StartTime = llvm::sys::TimeValue::now();
+    Mod->registerDeleteNotificationHandler(SFT);
+    if (breakBeforeRunning(F->getName(), SFT->getName()))
+      LLVM_BUILTIN_DEBUGTRAP;
+    SFT->run();
+    assert(analysesUnlocked() && "Expected all analyses to be unlocked!");
+    Mod->removeDeleteNotificationHandler(SFT);
+
+    // Did running the transform result in new functions being added
+    // to the top of our worklist?
+    bool newFunctionsAdded = (F != FunctionWorklist.back());
+
+    if (SILPrintPassTime) {
+      auto Delta =
+          llvm::sys::TimeValue::now().nanoseconds() - StartTime.nanoseconds();
+      llvm::dbgs() << Delta << " (" << SFT->getName() << "," << F->getName()
+                   << ")\n";
+    }
 
-  if (doPrintBefore(SFT, F)) {
-    llvm::dbgs() << "*** SIL function before " << StageName << " "
-                 << SFT->getName() << " (" << NumOptimizationIterations
-                 << ") ***\n";
-    F->dump(getOptions().EmitVerboseSIL);
-  }
+    // If this pass invalidated anything, print and verify.
+    if (doPrintAfter(SFT, F, CurrentPassHasInvalidated && SILPrintAll)) {
+      llvm::dbgs() << "*** SIL function after " << StageName << " "
+                   << SFT->getName() << " (" << NumOptimizationIterations
+                   << ") ***\n";
+      F->dump(Options.EmitVerboseSIL);
+    }
 
-  llvm::sys::TimeValue StartTime = llvm::sys::TimeValue::now();
-  Mod->registerDeleteNotificationHandler(SFT);
-  if (breakBeforeRunning(F->getName(), SFT->getName()))
-    LLVM_BUILTIN_DEBUGTRAP;
-  SFT->run();
-  assert(analysesUnlocked() && "Expected all analyses to be unlocked!");
-  Mod->removeDeleteNotificationHandler(SFT);
+    // Remember if this pass didn't change anything.
+    if (!CurrentPassHasInvalidated)
+      completedPasses.set((size_t)SFT->getPassKind());
 
-  if (SILPrintPassTime) {
-    auto Delta =
-        llvm::sys::TimeValue::now().nanoseconds() - StartTime.nanoseconds();
-    llvm::dbgs() << Delta << " (" << SFT->getName() << "," << F->getName()
-                 << ")\n";
-  }
+    if (Options.VerifyAll &&
+        (CurrentPassHasInvalidated || SILVerifyWithoutInvalidation)) {
+      F->verify();
+      verifyAnalyses(F);
+    }
 
-  // If this pass invalidated anything, print and verify.
-  if (doPrintAfter(SFT, F, CurrentPassHasInvalidated && SILPrintAll)) {
-    llvm::dbgs() << "*** SIL function after " << StageName << " "
-                 << SFT->getName() << " (" << NumOptimizationIterations
-                 << ") ***\n";
-    F->dump(getOptions().EmitVerboseSIL);
-  }
+    ++NumPassesRun;
+
+    if (!continueTransforming())
+      return;
 
-  // Remember if this pass didn't change anything.
-  if (!CurrentPassHasInvalidated)
-    completedPasses.set((size_t)SFT->getPassKind());
+    if (runToCompletion)
+      continue;
 
-  if (getOptions().VerifyAll &&
-      (CurrentPassHasInvalidated || SILVerifyWithoutInvalidation)) {
-    F->verify();
-    verifyAnalyses(F);
+    // If running the transform resulted in new functions on the top
+    // of the worklist, we'll return so that we can begin processing
+    // those new functions.
+    if (shouldRestartPipeline() || newFunctionsAdded)
+      return;
   }
-
-  ++NumPassesRun;
 }
 
 void SILPassManager::runFunctionPasses(PassList FuncTransforms) {
-
-  if (FuncTransforms.empty())
-    return;
-
   BasicCalleeAnalysis *BCA = getAnalysis<BasicCalleeAnalysis>();
   BottomUpFunctionOrder BottomUpOrder(*Mod, BCA);
   auto BottomUpFunctions = BottomUpOrder.getFunctions();
@@ -373,41 +395,82 @@ void SILPassManager::runFunctionPasses(PassList FuncTransforms) {
       FunctionWorklist.push_back(*I);
   }
 
-  // The maximum number of times the pass pipeline can be restarted for a
-  // function. This is used to ensure we are not going into an infinite loop in
-  // cases where (for example) we have recursive type-based specialization
+  // Used to track how many times a given function has been
+  // (partially) optimized by the function pass pipeline in this
+  // invocation.
+  llvm::DenseMap<SILFunction *, unsigned> CountOptimized;
+
+  // Count of how many iterations we've had since any function was
+  // popped off the function worklist. This is used to ensure progress
+  // and eliminate the chance of going into an infinite loop in cases
+  // where (for example) we have recursive type-based specialization
   // happening.
-  const unsigned MaxNumRestarts = 20;
+  unsigned IterationsWithoutProgress = 0;
 
-  if (SILPrintPassName)
-    llvm::dbgs() << "Start function passes at stage: " << StageName << "\n";
+  // The maximum number of functions we'll optimize without popping
+  // any off the worklist. This is expected to non-zero.
+  const unsigned MaxIterationsWithoutProgress = 20;
 
-  // Run all transforms for all functions, starting at the tail of the worklist.
+  // Pop functions off the worklist, and run all function transforms
+  // on each of them.
   while (!FunctionWorklist.empty() && continueTransforming()) {
-    unsigned TailIdx = FunctionWorklist.size() - 1;
-    unsigned PipelineIdx = FunctionWorklist[TailIdx].PipelineIdx;
-    SILFunction *F = FunctionWorklist[TailIdx].F;
+    auto *F = FunctionWorklist.back();
 
-    if (PipelineIdx >= FuncTransforms.size()) {
-      // All passes did already run for the function. Pop it off the worklist.
+    // If we've done many iterations without progress, pop the current
+    // function and any other function we've run any optimizations on
+    // on from the stack and then continue.
+    if (IterationsWithoutProgress == (MaxIterationsWithoutProgress - 1)) {
+      // Pop the current (potentially not-yet-optimized) function off.
       FunctionWorklist.pop_back();
+      IterationsWithoutProgress = 0;
+
+      // Pop any remaining functions that have been optimized (at
+      // least through some portion of the pipeline).
+      while (!FunctionWorklist.empty() &&
+             CountOptimized[FunctionWorklist.back()] > 0)
+        FunctionWorklist.pop_back();
+
       continue;
     }
-    assert(!shouldRestartPipeline() &&
+
+    if (CountOptimized[F] > SILFunctionPassPipelineLimit) {
+      DEBUG(llvm::dbgs() << "*** Hit limit optimizing: " << F->getName()
+                         << '\n');
+      FunctionWorklist.pop_back();
+      IterationsWithoutProgress = 0;
+      continue;
+    }
+
+    assert(
+        !shouldRestartPipeline() &&
         "Did not expect function pipeline set up to restart from beginning!");
 
-    runPassOnFunction(FuncTransforms[PipelineIdx], F);
+    assert(CountOptimized[F] <= SILFunctionPassPipelineLimit &&
+           "Function optimization count exceeds limit!");
+    auto runToCompletion = CountOptimized[F] == SILFunctionPassPipelineLimit;
 
-    // Note: Don't get entry reference prior to runPassOnFunction().
-    // A pass can push a new function to the worklist which may cause a
-    // reallocation of the buffer and that would invalidate the reference.
-    WorklistEntry &Entry = FunctionWorklist[TailIdx];
-    if (shouldRestartPipeline() && Entry.NumRestarts < MaxNumRestarts) {
-      ++Entry.NumRestarts;
-      Entry.PipelineIdx = 0;
-    } else {
-      ++Entry.PipelineIdx;
+    runPassesOnFunction(FuncTransforms, F, runToCompletion);
+    ++CountOptimized[F];
+    ++IterationsWithoutProgress;
+
+    if (runToCompletion) {
+      FunctionWorklist.pop_back();
+      IterationsWithoutProgress = 0;
+      clearRestartPipeline();
+      continue;
     }
+
+
+    // If running the function transforms did not result in new
+    // functions being added to the top of the worklist, then we're
+    // done with this function and can pop it off and continue.
+    // Otherwise, we'll return to this function and reoptimize after
+    // processing the new functions that were added.
+    if (F == FunctionWorklist.back() && !shouldRestartPipeline()) {
+      FunctionWorklist.pop_back();
+      IterationsWithoutProgress = 0;
+    }
+
     clearRestartPipeline();
   }
 }

lib/SILOptimizer/Transforms/FunctionSignatureOpts.cpp

@@ -869,13 +869,6 @@ class FunctionSignatureOpts : public SILFunctionTransform {
       // Make sure the PM knows about this function. This will also help us
       // with self-recursion.
       notifyPassManagerOfFunction(FST.getOptimizedFunction());
-
-      // We have to restart the pipeline for this thunk in order to run the
-      // inliner (and other opts) again. This is important if the new
-      // specialized function (which is called from this thunk) is
-      // function-signature-optimized again and also becomes an
-      // always-inline-thunk.
-      restartPassPipeline();
     }
   }
 

test/SILOptimizer/inline_devirtualize_specialize.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -enable-sil-verify-all %s -devirtualizer -generic-specializer -inline -dce | %FileCheck %s
+// RUN: %target-sil-opt -enable-sil-verify-all %s -inline -devirtualizer -generic-specializer -dce | %FileCheck %s
 
 sil_stage canonical