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

[fedor.sergeev]

Summary

IR-printing AfterPass instrumentation might be called on a loop
that has just been invalidated. We should skip printing it to
avoid spurious asserts.

[fedor.sergeev]

oops... scc-deleted-printer.ll is not yet ready - skip it until updated.
everything else should be fine.

[chandlerc]

include/llvm/IR/PassInstrumentation.h

160–162

I think this should have a different name to indicate that it is called in a fundamentally different situation -- after an IR unit is invalidated.

166–167

Similarly, I think the callbacks should be different. The user can always do this binding with nullptr if that's a desirable way to model it, but generally I think different names of routines seem more appropriate.

lib/Passes/StandardInstrumentations.cpp

136–137

FWIW, I think the way to handle this is to bind a reference to the module into the callback so that it intrinsically has access to it to print.

lib/Transforms/Scalar/LoopPassManager.cpp

47–51

nit: Here and elsewhere in comments: please use proper prose with capitalized first letter and punctuation.

[fedor.sergeev]

scc-deleted-printer test updated

[fedor.sergeev]

include/llvm/IR/PassInstrumentation.h

166–167

Since this nullptr hack is not under control of the user its either the same callback with nullptr (as of right now) or a callback w/o any IR at all.

Conceptually, using different callbacks does look like a clearner solution, though I'm a bit worried that we double the amount of callbacks for a somewhat limited added value...
Need to try it out and see how it looks like.

lib/Passes/StandardInstrumentations.cpp

136–137

Thats true, just wanted to do that separately (and, btw, address the same problem for Legacy printer).

[fedor.sergeev]

moved "invalidated after-pass" into its own callback, looks like a proper solution, indeed.

[fedor.sergeev]

adding new instrumentation point to unittests

[fedor.sergeev]

ping?

[philip.pfaffe]

lib/Passes/StandardInstrumentations.cpp

58–60

Is that still the case? You don't hook up the Invalidated callback below.

136–137

Same as above.

[fedor.sergeev]

lib/Passes/StandardInstrumentations.cpp

58–60

Hmm... indeed. I did it in the followup which handles -print-module-scope part and it has not been posted yet :-(
Okey, let me clean this up once more.

[chandlerc]

One place that I'm not seeing an update reflected (but I may just misunderstand)....

lib/Passes/StandardInstrumentations.cpp

58–60

Did this happen?

[fedor.sergeev]

lib/Passes/StandardInstrumentations.cpp

58–60

not yet, lagging behind

[fedor.sergeev]

slight cleanup in anticipation of a followup that handles invalidated printing

[philip.pfaffe]

lib/Passes/StandardInstrumentations.cpp

88

Is there a policy on assertions?

Those you added imho don't add value. You're asserting for nullptr, but are using the pointer unconditionally after. That's failing just as hard. The message on the other hand isn't very helpful I feel, since you're not actually testing loop validity, but whether a nullptr was stored in the any, which I don't think is the same thing (anymore?).

[fedor.sergeev]

lib/Passes/StandardInstrumentations.cpp

88

Well... I have no idea about policies on assertions.

To me a very message of assert is usually considerably less helpful than the location of it.
Usually I dont read the message and just jump to the source when it hits.

You are right that now its not checking for validity, and it remains there only for the check against accidental nullptr (read - but in PassManager).
So yes, I might just scrap these...

[fedor.sergeev]

removing unneeded asserts

[philip.pfaffe]

Thanks, this does LGTM!

One additional thought though, and if this doesn't make sense to you feel free to commit:

In case the IRUnit has been invalidated, can we still pass it to the callback? When I think of instrumentations that do more complicated things than printing, it's highly likely that they would like to be notified when an IRUnit handle becomes invalid. Which requires passing the handle. If we are passing the handle, though, we need to emphasize that the unit isn't accessible through the handle anymore. Maybe by passing void* instead, or some tag type Invalidated<T> { void* Handle; }. The latter can go through the any and allow deducing the actual IRUnit type.

[fedor.sergeev]

In D54740#1325291, @philip.pfaffe wrote:

In case the IRUnit has been invalidated, can we still pass it to the callback? When I think of instrumentations that do more complicated things than printing, it's highly likely that they would like to be notified when an IRUnit handle becomes invalid. Which requires passing the handle. If we are passing the handle, though, we need to emphasize that the unit isn't accessible through the handle anymore. Maybe by passing void* instead, or some tag type Invalidated<T> { void* Handle; }. The latter can go through the any and allow deducing the actual IRUnit type.

That would require putting more order on what *is* the invalidated IRUnit.
Overall this looks like a fine solution that can handle come complicated cases, however I would rather follow a common approach and wait until the need for that complication arises.
Perhaps will add a comment somewhere in PassInstrumentation that hints about this.

[philip.pfaffe]

The IRUnit is invalid, but the handle still exists, right?

Anyways, happy to do this as soon as this becomes an issue :)

[fedor.sergeev]

In D54740#1326123, @philip.pfaffe wrote:

The IRUnit is invalid, but the handle still exists, right?

Invalid handle might be totally invalid, you can not use it for anything.
I do remember having accesses to freed memory when accessing either Loop or SCC, dont remember which one.
Even looking at the address might be not that sane if it was freed and perhaps something else allocated at that place.

Anyway, my point is that we do not have a well established notion of invalid IR.
And solution that tries to put order into pass-instrumentation handling of invalid IR first
needs to put order into overall handling of that invalid IR.

[philip.pfaffe]

The invalid handle isn't good for accessing anything, but it can still identify a unit.

[fedor.sergeev]

In D54740#1326165, @philip.pfaffe wrote:

The invalid handle isn't good for accessing anything, but it can still identify a unit.

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

[philip.pfaffe]

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

The point is what kind of information you attach to the handle. When triggering the invalidation callbacks, the message is "This particular loop was invalidated". And that's a valid message, no matter if the handle is already being reused, because a downstream instrumentation cannot have observed the new loop behind the handle yet since we're still on the invalidation path.

include/llvm/Analysis/CGSCCPassManager.h

for (auto RCI = CG.postorder_ref_scc_begin(),
             // Check the PassInstrumentation's BeforePass callbacks before
             // running the pass, skip its execution completely if asked to
             // (callback returns false).
             if (!PI.runBeforePass<LazyCallGraph::SCC>(Pass, *C))
               continue;
 
             PreservedAnalyses PassPA = Pass.run(*C, CGAM, CG, UR);
 
+            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;
             RC = UR.UpdatedRC ? UR.UpdatedRC : RC;
 
             // If the CGSCC pass wasn't able to provide a valid updated SCC,
             // the current SCC may simply need to be skipped if invalid.
             if (UR.InvalidatedSCCs.count(C)) {
PreservedAnalyses run(LazyCallGraph::SCC &InitialC, CGSCCAnalysisManager &AM,
 
     for (int Iteration = 0;; ++Iteration) {
 
       if (!PI.runBeforePass<LazyCallGraph::SCC>(Pass, *C))
         continue;
 
       PreservedAnalyses PassPA = Pass.run(*C, AM, CG, UR);
 
+      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.
       if (UR.UpdatedC && UR.UpdatedC != C) {
         PA.intersect(std::move(PassPA));
         break;
       }
 

include/llvm/IR/PassInstrumentation.h

 namespace llvm {
 
 class PreservedAnalyses;
 
 /// This class manages callbacks registration, as well as provides a way for
 /// 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.
   using BeforePassFunc = bool(StringRef, Any);
   using AfterPassFunc = void(StringRef, Any);
+  using AfterPassInvalidatedFunc = void(StringRef);
   using BeforeAnalysisFunc = void(StringRef, Any);
   using AfterAnalysisFunc = void(StringRef, Any);
 
 public:
   PassInstrumentationCallbacks() {}
 
   /// Copying PassInstrumentationCallbacks is not intended.
   PassInstrumentationCallbacks(const PassInstrumentationCallbacks &) = delete;
   void operator=(const PassInstrumentationCallbacks &) = delete;
 
   template <typename CallableT> void registerBeforePassCallback(CallableT C) {
     BeforePassCallbacks.emplace_back(std::move(C));
   }
 
   template <typename CallableT> void registerAfterPassCallback(CallableT C) {
     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));
   }
 
   template <typename CallableT>
   void registerAfterAnalysisCallback(CallableT C) {
     AfterAnalysisCallbacks.emplace_back(std::move(C));
   }
 
 private:
   friend class PassInstrumentation;
 
   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>
       AfterAnalysisCallbacks;
 };
 
 /// This class provides instrumentation entry points for the Pass Manager,
 /// doing calls to callbacks registered in PassInstrumentationCallbacks.
bool runBeforePass(const PassT &Pass, const IRUnitT &IR) const {
 
     bool ShouldRun = true;
     for (auto &C : Callbacks->BeforePassCallbacks)
       ShouldRun &= C(Pass.name(), llvm::Any(&IR));
     return ShouldRun;
   }
 
   /// 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.
   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>
   void runBeforeAnalysis(const PassT &Analysis, const IRUnitT &IR) const {
     if (Callbacks)
       for (auto &C : Callbacks->BeforeAnalysisCallbacks)
         C(Analysis.name(), llvm::Any(&IR));
   }

include/llvm/IR/PassTimingInfo.h

private:
 
   /// Returns the incremented counter for the next invocation of \p PassID.
   unsigned nextPassID(StringRef PassID) { return ++PassIDCountMap[PassID]; }
 
   void startTimer(StringRef PassID);
   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
 
 #endif

include/llvm/Transforms/Scalar/LoopPassManager.h

 #endif
       // Check the PassInstrumentation's BeforePass callbacks before running the
       // pass, skip its execution completely if asked to (callback returns
       // false).
       if (!PI.runBeforePass<Loop>(Pass, *L))
         continue;
       PreservedAnalyses PassPA = Pass.run(*L, LAM, LAR, Updater);
 
+      // 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.
 
       // If the loop hasn't been deleted, we need to handle invalidation here.
       if (!Updater.skipCurrentLoop())
         // We know that the loop pass couldn't have invalidated any other
         // loop's analyses (that's the contract of a loop pass), so directly

lib/Analysis/CGSCCPassManager.cpp

for (auto &Pass : Passes) {
 
     // Check the PassInstrumentation's BeforePass callbacks before running the
     // pass, skip its execution completely if asked to (callback returns false).
     if (!PI.runBeforePass(*Pass, *C))
       continue;
 
     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;
 
     // If the CGSCC pass wasn't able to provide a valid updated SCC, the
     // current SCC may simply need to be skipped if invalid.
     if (UR.InvalidatedSCCs.count(C)) {
       LLVM_DEBUG(dbgs() << "Skipping invalidated root or island SCC!\n");

lib/IR/PassTimingInfo.cpp

static bool matchPassManager(StringRef PassID) {
   size_t prefix_pos = PassID.find('<');
   if (prefix_pos == StringRef::npos)
     return false;
   StringRef Prefix = PassID.substr(0, prefix_pos);
   return Prefix.endswith("PassManager") || Prefix.endswith("PassAdaptor") ||
          Prefix.endswith("AnalysisManagerProxy");
 }
 
-bool TimePassesHandler::runBeforePass(StringRef PassID, Any IR) {
+bool TimePassesHandler::runBeforePass(StringRef PassID) {
   if (matchPassManager(PassID))
     return true;
 
   startTimer(PassID);
 
   LLVM_DEBUG(dbgs() << "after runBeforePass(" << PassID << ")\n");
   LLVM_DEBUG(dump());
 
   // we are not going to skip this pass, thus return true.
   return true;
 }
 
-void TimePassesHandler::runAfterPass(StringRef PassID, Any IR) {
+void TimePassesHandler::runAfterPass(StringRef PassID) {
   if (matchPassManager(PassID))
     return;
 
   stopTimer(PassID);
 
   LLVM_DEBUG(dbgs() << "after runAfterPass(" << PassID << ")\n");
   LLVM_DEBUG(dump());
 }
 
 void TimePassesHandler::registerCallbacks(PassInstrumentationCallbacks &PIC) {
   if (!Enabled)
     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

lib/Passes/StandardInstrumentations.cpp

 
 /// Generic IR-printing helper that unpacks a pointer to IRUnit wrapped into
 /// llvm::Any and does actual print job.
 void unwrapAndPrint(StringRef Banner, Any IR) {
   SmallString<40> Extra{"\n"};
   const Module *M = nullptr;
   if (any_isa<const Module *>(IR)) {
     M = any_cast<const Module *>(IR);
+    assert(M && "module should be valid for printing");
   } else if (any_isa<const Function *>(IR)) {
     const Function *F = any_cast<const Function *>(IR);
+    assert(F && "function should be valid for printing");
     if (!llvm::isFunctionInPrintList(F->getName()))
       return;
     if (!llvm::forcePrintModuleIR()) {
       dbgs() << Banner << Extra << static_cast<const Value &>(*F);
       return;
     }
     M = F->getParent();
     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);
+    // We get nullptr here when current SCC gets invalidated by the pass.
+    if (C == nullptr)
+      return;
     if (!llvm::forcePrintModuleIR()) {
       Extra = formatv(" (scc: {0})\n", C->getName());
       bool BannerPrinted = false;
       for (const LazyCallGraph::Node &N : *C) {
         const Function &F = N.getFunction();
         if (!F.isDeclaration() && isFunctionInPrintList(F.getName())) {
           if (!BannerPrinted) {
             dbgs() << Banner << Extra;
for (const LazyCallGraph::Node &N : *C) {
         break;
       }
     }
     if (!M)
       return;
     Extra = formatv(" (for scc: {0})\n", C->getName());
   } else if (any_isa<const Loop *>(IR)) {
     const Loop *L = any_cast<const Loop *>(IR);
+    // Current loop might have been invalidated by the pass,
+    // then we will get nullptr here.
+    if (L == nullptr)
+      return;
+
     const Function *F = L->getHeader()->getParent();
     if (!isFunctionInPrintList(F->getName()))
       return;
     if (!llvm::forcePrintModuleIR()) {
       llvm::printLoop(const_cast<Loop &>(*L), dbgs(), Banner);
       return;
     }
     M = F->getParent();

 void printAfterPass(StringRef PassID, Any IR) {
   if (!llvm::shouldPrintAfterPass(PassID))
     return;
 
   if (PassID.startswith("PassManager<") || PassID.contains("PassAdaptor<"))
     return;
 
   SmallString<20> Banner = formatv("*** IR Dump After {0} ***", PassID);
+  // TODO: handle cases when IR is nullptr (i.e. has been invalidated)
+  // but we still need to print the module due to forcePrintModuleIR.
   unwrapAndPrint(Banner, IR);
   return;
 }
 } // namespace PrintIR
 } // namespace
 
 void StandardInstrumentations::registerCallbacks(
     PassInstrumentationCallbacks &PIC) {
   if (llvm::shouldPrintBeforePass())
     PIC.registerBeforePassCallback(PrintIR::printBeforePass);
   if (llvm::shouldPrintAfterPass())
     PIC.registerAfterPassCallback(PrintIR::printAfterPass);
   TimePasses.registerCallbacks(PIC);
 }

lib/Transforms/Scalar/LoopPassManager.cpp

for (auto &Pass : Passes) {
 
     // Check the PassInstrumentation's BeforePass callbacks before running the
     // pass, skip its execution completely if asked to (callback returns false).
     if (!PI.runBeforePass<Loop>(*Pass, L))
       continue;
 
     PreservedAnalyses PassPA = Pass->run(L, AM, AR, U);
 
+    // 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()) {
       PA.intersect(std::move(PassPA));
       break;
     }
 
 #ifndef NDEBUG

test/Other/loop-deletion-printer.ll

+; 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
+}
+

test/Other/scc-deleted-printer.ll

+; 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
+}