[llvm] [CGSCC] Remove CGSCCUpdateResult::InvalidatedRefSCCs (PR #98213)
Arthur Eubanks via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 9 16:17:15 PDT 2024
https://github.com/aeubanks updated https://github.com/llvm/llvm-project/pull/98213
>From b92fbf9cff096802c30bbc40cc53ef0ec22ff92c Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Tue, 9 Jul 2024 20:07:41 +0000
Subject: [PATCH 1/2] [CGSCC] Remove CGSCCUpdateResult::InvalidatedRefSCCs
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.
---
llvm/include/llvm/Analysis/CGSCCPassManager.h | 8 --------
llvm/lib/Analysis/CGSCCPassManager.cpp | 10 +---------
llvm/lib/Transforms/Utils/CallGraphUpdater.cpp | 2 --
3 files changed, 1 insertion(+), 19 deletions(-)
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
>From 24c2095527fa29184c168acc9c8e0e76b7bdf93b Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Tue, 9 Jul 2024 23:16:50 +0000
Subject: [PATCH 2/2] Add test, fix CallGraphUpdater more
---
.../lib/Transforms/Utils/CallGraphUpdater.cpp | 8 +--
.../Analysis/CGSCCPassManagerTest.cpp | 55 +++++++++++++++++++
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
index 2ddb1e54075c6..7cba4829d4839 100644
--- a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
+++ b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
@@ -60,17 +60,13 @@ bool CallGraphUpdater::finalize() {
assert(DeadSCC && DeadSCC->size() == 1 &&
&DeadSCC->begin()->getFunction() == DeadFn);
- FunctionAnalysisManager &FAM =
- AM->getResult<FunctionAnalysisManagerCGSCCProxy>(*DeadSCC, *LCG)
- .getManager();
-
- FAM.clear(*DeadFn, DeadFn->getName());
+ FAM->clear(*DeadFn, DeadFn->getName());
AM->clear(*DeadSCC, DeadSCC->getName());
LCG->markDeadFunction(*DeadFn);
// Mark the relevant parts of the call graph as invalid so we don't
// visit them.
- UR->InvalidatedSCCs.insert(DeadSCC);
+ UR->InvalidatedSCCs.insert(LCG->lookupSCC(N));
UR->DeadFunctions.push_back(DeadFn);
} else {
// The CGSCC infrastructure batch deletes functions at the end of the
diff --git a/llvm/unittests/Analysis/CGSCCPassManagerTest.cpp b/llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
index 1532379c9ba02..9fd782d1b7614 100644
--- a/llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
+++ b/llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
@@ -1878,6 +1878,61 @@ TEST_F(CGSCCPassManagerTest, TestInsertionOfNewFunctions2) {
ASSERT_TRUE(Ran);
}
+TEST_F(CGSCCPassManagerTest, TestDeletionOfFunctionInNonTrivialRefSCC) {
+ std::unique_ptr<Module> M = parseIR("define void @f1() {\n"
+ "entry:\n"
+ " call void @f2()\n"
+ " ret void\n"
+ "}\n"
+ "define void @f2() {\n"
+ "entry:\n"
+ " call void @f1()\n"
+ " ret void\n"
+ "}\n");
+
+ bool Ran = false;
+ CGSCCPassManager CGPM;
+ CGPM.addPass(LambdaSCCPassNoPreserve(
+ [&](LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM, LazyCallGraph &CG,
+ CGSCCUpdateResult &UR) {
+ if (Ran)
+ return;
+
+ LazyCallGraph::Node *N1 = nullptr;
+
+ for (LazyCallGraph::Node *N : SCCNodes(C)) {
+ Function &F = N->getFunction();
+ if (F.getName() != "f1")
+ continue;
+ N1 = N;
+
+ Function &F2 = *F.getParent()->getFunction("f2");
+
+ // Remove f1 <-> f2 references
+ F.getEntryBlock().front().eraseFromParent();
+ F2.getEntryBlock().front().eraseFromParent();
+
+ CallGraphUpdater CGU;
+ CGU.initialize(CG, C, AM, UR);
+ CGU.removeFunction(F2);
+ CGU.reanalyzeFunction(F);
+
+ Ran = true;
+ }
+
+ // Check that updateCGAndAnalysisManagerForCGSCCPass() after
+ // CallGraphUpdater::removeFunction() succeeds.
+ updateCGAndAnalysisManagerForCGSCCPass(CG, *CG.lookupSCC(*N1), *N1, AM,
+ UR, FAM);
+ }));
+
+ ModulePassManager MPM;
+ MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
+ MPM.run(*M, MAM);
+
+ ASSERT_TRUE(Ran);
+}
+
TEST_F(CGSCCPassManagerTest, TestInsertionOfNewNonTrivialCallEdge) {
std::unique_ptr<Module> M = parseIR("define void @f1() {\n"
"entry:\n"
More information about the llvm-commits
mailing list