[CGSCC] Remove CGSCCUpdateResult::InvalidatedRefSCCs

[aeubanks]

The RefSCC that a function marked dead is in may still contain valid SCCs that we want to visit. We rely on InvalidatedSCCs to skip SCCs containing dead functions.

The addition of RefSCCs in CallGraphUpdater to InvalidatedRefSCCs was causing asserts as reported in #94815. Fix some more CallGraphUpdater function deletion methods as well.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Arthur Eubanks (aeubanks)

Changes

The RefSCC that a function marked dead is in may still contain valid SCCs that we want to visit. We rely on InvalidatedSCCs to skip SCCs containing dead functions.

The addition of RefSCCs in CallGraphUpdater to InvalidatedRefSCCs was causing asserts as reported in #94815. I'm having trouble coming up with a reduced reproducer and plan on ripping out CallGraphUpdater anyway (since its purpose is to support both the legacy pass manager and new pass manager CGSCC infra, and the legacy pass manager CGSCC is no longer used), so no test.

---

Full diff: https://github.com/llvm/llvm-project/pull/98213.diff

3 Files Affected:

• (modified) llvm/include/llvm/Analysis/CGSCCPassManager.h (-8)
• (modified) llvm/lib/Analysis/CGSCCPassManager.cpp (+1-9)
• (modified) llvm/lib/Transforms/Utils/CallGraphUpdater.cpp (-2)

diff --git a/llvm/include/llvm/Analysis/CGSCCPassManager.h b/llvm/include/llvm/Analysis/CGSCCPassManager.h
index 59974d146b56d..406d3492136fc 100644
--- a/llvm/include/llvm/Analysis/CGSCCPassManager.h
+++ b/llvm/include/llvm/Analysis/CGSCCPassManager.h
@@ -245,14 +245,6 @@ struct CGSCCUpdateResult {
   /// in reverse post-order.
   SmallPriorityWorklist<LazyCallGraph::SCC *, 1> &CWorklist;
 
-  /// The set of invalidated RefSCCs which should be skipped if they are found
-  /// in \c RCWorklist.
-  ///
-  /// This is used to quickly prune out RefSCCs when they get deleted and
-  /// happen to already be on the worklist. We use this primarily to avoid
-  /// scanning the list and removing entries from it.
-  SmallPtrSetImpl<LazyCallGraph::RefSCC *> &InvalidatedRefSCCs;
-
   /// The set of invalidated SCCs which should be skipped if they are found
   /// in \c CWorklist.
   ///
diff --git a/llvm/lib/Analysis/CGSCCPassManager.cpp b/llvm/lib/Analysis/CGSCCPassManager.cpp
index 24b3c6eef1084..c32739a565541 100644
--- a/llvm/lib/Analysis/CGSCCPassManager.cpp
+++ b/llvm/lib/Analysis/CGSCCPassManager.cpp
@@ -150,9 +150,8 @@ ModuleToPostOrderCGSCCPassAdaptor::run(Module &M, ModuleAnalysisManager &AM) {
   SmallPriorityWorklist<LazyCallGraph::RefSCC *, 1> RCWorklist;
   SmallPriorityWorklist<LazyCallGraph::SCC *, 1> CWorklist;
 
-  // Keep sets for invalidated SCCs and RefSCCs that should be skipped when
+  // Keep sets for invalidated SCCs that should be skipped when
   // iterating off the worklists.
-  SmallPtrSet<LazyCallGraph::RefSCC *, 4> InvalidRefSCCSet;
   SmallPtrSet<LazyCallGraph::SCC *, 4> InvalidSCCSet;
 
   SmallDenseSet<std::pair<LazyCallGraph::Node *, LazyCallGraph::SCC *>, 4>
@@ -161,7 +160,6 @@ ModuleToPostOrderCGSCCPassAdaptor::run(Module &M, ModuleAnalysisManager &AM) {
   SmallVector<Function *, 4> DeadFunctions;
 
   CGSCCUpdateResult UR = {CWorklist,
-                          InvalidRefSCCSet,
                           InvalidSCCSet,
                           nullptr,
                           PreservedAnalyses::all(),
@@ -194,11 +192,6 @@ ModuleToPostOrderCGSCCPassAdaptor::run(Module &M, ModuleAnalysisManager &AM) {
 
     do {
       LazyCallGraph::RefSCC *RC = RCWorklist.pop_back_val();
-      if (InvalidRefSCCSet.count(RC)) {
-        LLVM_DEBUG(dbgs() << "Skipping an invalid RefSCC...\n");
-        continue;
-      }
-
       assert(CWorklist.empty() &&
              "Should always start with an empty SCC worklist");
 
@@ -1172,7 +1165,6 @@ static LazyCallGraph::SCC &updateCGAndAnalysisManagerForPass(
   }
 
   assert(!UR.InvalidatedSCCs.count(C) && "Invalidated the current SCC!");
-  assert(!UR.InvalidatedRefSCCs.count(RC) && "Invalidated the current RefSCC!");
   assert(&C->getOuterRefSCC() == RC && "Current SCC not in current RefSCC!");
 
   // Record the current SCC for higher layers of the CGSCC pass manager now that
diff --git a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
index e9f37d4044cb0..2ddb1e54075c6 100644
--- a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
+++ b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
@@ -59,7 +59,6 @@ bool CallGraphUpdater::finalize() {
         auto *DeadSCC = LCG->lookupSCC(N);
         assert(DeadSCC && DeadSCC->size() == 1 &&
                &DeadSCC->begin()->getFunction() == DeadFn);
-        auto &DeadRC = DeadSCC->getOuterRefSCC();
 
         FunctionAnalysisManager &FAM =
             AM->getResult<FunctionAnalysisManagerCGSCCProxy>(*DeadSCC, *LCG)
@@ -72,7 +71,6 @@ bool CallGraphUpdater::finalize() {
         // Mark the relevant parts of the call graph as invalid so we don't
         // visit them.
         UR->InvalidatedSCCs.insert(DeadSCC);
-        UR->InvalidatedRefSCCs.insert(&DeadRC);
         UR->DeadFunctions.push_back(DeadFn);
       } else {
         // The CGSCC infrastructure batch deletes functions at the end of the

[aeubanks]

actually I do have a unittest that repros the assertion, will add that