[NewPM] fixing asserts on deleted loop in -print-after-all

IR-printing AfterPass instrumentation might be called on a loop that has just been invalidated. We should skip printing it to avoid spurious asserts. Reviewed By: chandlerc, philip.pfaffe Differential Revision: https://reviews.llvm.org/D54740 llvm-svn: 348887
llvm · Dec 11, 2018 · a1d95c3 · a1d95c3
1 parent 8876dac
commit a1d95c3
Show file tree

Hide file tree

Showing 11 changed files with 233 additions and 17 deletions.
diff --git a/llvm/include/llvm/Analysis/CGSCCPassManager.h b/llvm/include/llvm/Analysis/CGSCCPassManager.h
@@ -441,7 +441,10 @@ class ModuleToPostOrderCGSCCPassAdaptor
 
             PreservedAnalyses PassPA = Pass.run(*C, CGAM, CG, UR);
 
-            PI.runAfterPass<LazyCallGraph::SCC>(Pass, *C);
+            if (UR.InvalidatedSCCs.count(C))
+              PI.runAfterPassInvalidated<LazyCallGraph::SCC>(Pass);
+            else
+              PI.runAfterPass<LazyCallGraph::SCC>(Pass, *C);
 
             // Update the SCC and RefSCC if necessary.
             C = UR.UpdatedC ? UR.UpdatedC : C;
@@ -762,7 +765,10 @@ class DevirtSCCRepeatedPass
 
       PreservedAnalyses PassPA = Pass.run(*C, AM, CG, UR);
 
-      PI.runAfterPass<LazyCallGraph::SCC>(Pass, *C);
+      if (UR.InvalidatedSCCs.count(C))
+        PI.runAfterPassInvalidated<LazyCallGraph::SCC>(Pass);
+      else
+        PI.runAfterPass<LazyCallGraph::SCC>(Pass, *C);
 
       // If the SCC structure has changed, bail immediately and let the outer
       // CGSCC layer handle any iteration to reflect the refined structure.

diff --git a/llvm/include/llvm/IR/PassInstrumentation.h b/llvm/include/llvm/IR/PassInstrumentation.h
@@ -68,10 +68,17 @@ class PreservedAnalyses;
 /// PassInstrumentation to pass control to the registered callbacks.
 class PassInstrumentationCallbacks {
 public:
-  // Before/After callbacks accept IRUnits, so they need to take them
-  // as pointers, wrapped with llvm::Any
+  // Before/After callbacks accept IRUnits whenever appropriate, so they need
+  // to take them as constant pointers, wrapped with llvm::Any.
+  // For the case when IRUnit has been invalidated there is a different
+  // callback to use - AfterPassInvalidated.
+  // TODO: currently AfterPassInvalidated does not accept IRUnit, since passing
+  // already invalidated IRUnit is unsafe. There are ways to handle invalidated IRUnits
+  // in a safe way, and we might pursue that as soon as there is a useful instrumentation
+  // that needs it.
   using BeforePassFunc = bool(StringRef, Any);
   using AfterPassFunc = void(StringRef, Any);
+  using AfterPassInvalidatedFunc = void(StringRef);
   using BeforeAnalysisFunc = void(StringRef, Any);
   using AfterAnalysisFunc = void(StringRef, Any);
 
@@ -90,6 +97,11 @@ class PassInstrumentationCallbacks {
     AfterPassCallbacks.emplace_back(std::move(C));
   }
 
+  template <typename CallableT>
+  void registerAfterPassInvalidatedCallback(CallableT C) {
+    AfterPassInvalidatedCallbacks.emplace_back(std::move(C));
+  }
+
   template <typename CallableT>
   void registerBeforeAnalysisCallback(CallableT C) {
     BeforeAnalysisCallbacks.emplace_back(std::move(C));
@@ -105,6 +117,8 @@ class PassInstrumentationCallbacks {
 
   SmallVector<llvm::unique_function<BeforePassFunc>, 4> BeforePassCallbacks;
   SmallVector<llvm::unique_function<AfterPassFunc>, 4> AfterPassCallbacks;
+  SmallVector<llvm::unique_function<AfterPassInvalidatedFunc>, 4>
+      AfterPassInvalidatedCallbacks;
   SmallVector<llvm::unique_function<BeforeAnalysisFunc>, 4>
       BeforeAnalysisCallbacks;
   SmallVector<llvm::unique_function<AfterAnalysisFunc>, 4>
@@ -139,14 +153,25 @@ class PassInstrumentation {
   }
 
   /// AfterPass instrumentation point - takes \p Pass instance that has
-  /// just been executed and constant reference to IR it operates on.
+  /// just been executed and constant reference to \p IR it operates on.
+  /// \p IR is guaranteed to be valid at this point.
   template <typename IRUnitT, typename PassT>
   void runAfterPass(const PassT &Pass, const IRUnitT &IR) const {
     if (Callbacks)
       for (auto &C : Callbacks->AfterPassCallbacks)
         C(Pass.name(), llvm::Any(&IR));
   }
 
+  /// AfterPassInvalidated instrumentation point - takes \p Pass instance
+  /// that has just been executed. For use when IR has been invalidated
+  /// by \p Pass execution.
+  template <typename IRUnitT, typename PassT>
+  void runAfterPassInvalidated(const PassT &Pass) const {
+    if (Callbacks)
+      for (auto &C : Callbacks->AfterPassInvalidatedCallbacks)
+        C(Pass.name());
+  }
+
   /// BeforeAnalysis instrumentation point - takes \p Analysis instance
   /// to be executed and constant reference to IR it operates on.
   template <typename IRUnitT, typename PassT>

diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h
@@ -99,8 +99,8 @@ class TimePassesHandler {
   void stopTimer(StringRef PassID);
 
   // Implementation of pass instrumentation callbacks.
-  bool runBeforePass(StringRef PassID, Any IR);
-  void runAfterPass(StringRef PassID, Any IR);
+  bool runBeforePass(StringRef PassID);
+  void runAfterPass(StringRef PassID);
 };
 
 } // namespace llvm

diff --git a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
@@ -352,7 +352,11 @@ class FunctionToLoopPassAdaptor
         continue;
       PreservedAnalyses PassPA = Pass.run(*L, LAM, LAR, Updater);
 
-      PI.runAfterPass<Loop>(Pass, *L);
+      // Do not pass deleted Loop into the instrumentation.
+      if (Updater.skipCurrentLoop())
+        PI.runAfterPassInvalidated<Loop>(Pass);
+      else
+        PI.runAfterPass<Loop>(Pass, *L);
 
       // FIXME: We should verify the set of analyses relevant to Loop passes
       // are preserved.

diff --git a/llvm/lib/Analysis/CGSCCPassManager.cpp b/llvm/lib/Analysis/CGSCCPassManager.cpp
@@ -79,7 +79,10 @@ PassManager<LazyCallGraph::SCC, CGSCCAnalysisManager, LazyCallGraph &,
 
     PreservedAnalyses PassPA = Pass->run(*C, AM, G, UR);
 
-    PI.runAfterPass(*Pass, *C);
+    if (UR.InvalidatedSCCs.count(C))
+      PI.runAfterPassInvalidated<LazyCallGraph::SCC>(*Pass);
+    else
+      PI.runAfterPass<LazyCallGraph::SCC>(*Pass, *C);
 
     // Update the SCC if necessary.
     C = UR.UpdatedC ? UR.UpdatedC : C;

diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp
@@ -226,7 +226,7 @@ static bool matchPassManager(StringRef PassID) {
          Prefix.endswith("AnalysisManagerProxy");
 }
 
-bool TimePassesHandler::runBeforePass(StringRef PassID, Any IR) {
+bool TimePassesHandler::runBeforePass(StringRef PassID) {
   if (matchPassManager(PassID))
     return true;
 
@@ -239,7 +239,7 @@ bool TimePassesHandler::runBeforePass(StringRef PassID, Any IR) {
   return true;
 }
 
-void TimePassesHandler::runAfterPass(StringRef PassID, Any IR) {
+void TimePassesHandler::runAfterPass(StringRef PassID) {
   if (matchPassManager(PassID))
     return;
 
@@ -254,13 +254,15 @@ void TimePassesHandler::registerCallbacks(PassInstrumentationCallbacks &PIC) {
     return;
 
   PIC.registerBeforePassCallback(
-      [this](StringRef P, Any IR) { return this->runBeforePass(P, IR); });
+      [this](StringRef P, Any) { return this->runBeforePass(P); });
   PIC.registerAfterPassCallback(
-      [this](StringRef P, Any IR) { this->runAfterPass(P, IR); });
+      [this](StringRef P, Any) { this->runAfterPass(P); });
+  PIC.registerAfterPassInvalidatedCallback(
+      [this](StringRef P) { this->runAfterPass(P); });
   PIC.registerBeforeAnalysisCallback(
-      [this](StringRef P, Any IR) { this->runBeforePass(P, IR); });
+      [this](StringRef P, Any) { this->runBeforePass(P); });
   PIC.registerAfterAnalysisCallback(
-      [this](StringRef P, Any IR) { this->runAfterPass(P, IR); });
+      [this](StringRef P, Any) { this->runAfterPass(P); });
 }
 
 } // namespace llvm
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -53,7 +53,6 @@ void unwrapAndPrint(StringRef Banner, Any IR) {
     Extra = formatv(" (function: {0})\n", F->getName());
   } else if (any_isa<const LazyCallGraph::SCC *>(IR)) {
     const LazyCallGraph::SCC *C = any_cast<const LazyCallGraph::SCC *>(IR);
-    assert(C);
     if (!llvm::forcePrintModuleIR()) {
       Extra = formatv(" (scc: {0})\n", C->getName());
       bool BannerPrinted = false;

diff --git a/llvm/lib/Transforms/Scalar/LoopPassManager.cpp b/llvm/lib/Transforms/Scalar/LoopPassManager.cpp
@@ -44,7 +44,11 @@ PassManager<Loop, LoopAnalysisManager, LoopStandardAnalysisResults &,
 
     PreservedAnalyses PassPA = Pass->run(L, AM, AR, U);
 
-    PI.runAfterPass<Loop>(*Pass, L);
+    // do not pass deleted Loop into the instrumentation
+    if (U.skipCurrentLoop())
+      PI.runAfterPassInvalidated<Loop>(*Pass);
+    else
+      PI.runAfterPass<Loop>(*Pass, L);
 
     // If the loop was deleted, abort the run and return to the outer walk.
     if (U.skipCurrentLoop()) {

diff --git a/llvm/test/Other/loop-deletion-printer.ll b/llvm/test/Other/loop-deletion-printer.ll
@@ -0,0 +1,24 @@
+; Make sure that Loop which was invalidated by loop-deletion
+; does not lead to problems for -print-after-all and is just skipped.
+;
+; RUN: opt < %s -disable-output \
+; RUN:     -passes=loop-instsimplify -print-after-all  2>&1 | FileCheck %s -check-prefix=SIMPLIFY
+; RUN: opt < %s -disable-output \
+; RUN:     -passes=loop-deletion,loop-instsimplify -print-after-all  2>&1 | FileCheck %s -check-prefix=DELETED
+;
+; SIMPLIFY: IR Dump {{.*}} LoopInstSimplifyPass
+; DELETED-NOT: IR Dump {{.*}}LoopInstSimplifyPass
+; DELETED-NOT: IR Dump {{.*}}LoopDeletionPass
+
+define void @deleteme() {
+entry:
+  br label %loop
+loop:
+  %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]
+  %iv.next = add i32 %iv, 1
+  %check = icmp ult i32 %iv.next, 3
+  br i1 %check, label %loop, label %exit
+exit:
+  ret void
+}
+
diff --git a/llvm/test/Other/scc-deleted-printer.ll b/llvm/test/Other/scc-deleted-printer.ll
@@ -0,0 +1,25 @@
+; RUN: opt < %s 2>&1 -disable-output \
+; RUN: 	   -passes=inline -print-before-all -print-after-all | FileCheck %s -check-prefix=INL
+; RUN: opt < %s 2>&1 -disable-output \
+; RUN: 	   -passes=inline -print-before-all -print-after-all -print-module-scope | FileCheck %s -check-prefix=INL-MOD
+
+; INL: IR Dump Before {{InlinerPass .*scc: .tester, foo}}
+; INL-NOT: IR Dump After {{InlinerPass}}
+; INL: IR Dump Before {{InlinerPass .*scc: .tester}}
+; INL: IR Dump After {{InlinerPass .*scc: .tester}}
+
+; INL-MOD: IR Dump Before {{InlinerPass .*scc: .tester, foo}}
+; INL-MOD-NOT: IR Dump After {{InlinerPass}}
+; INL-MOD: IR Dump Before {{InlinerPass .*scc: .tester}}
+; INL-MOD: IR Dump After {{InlinerPass .*scc: .tester}}
+
+
+define void @tester() noinline {
+  call void @foo()
+  ret void
+}
+
+define internal void @foo() alwaysinline {
+  call void @tester()
+  ret void
+}