[CallGraphUpdater] Remove some legacy pass manager support

[aeubanks]

We don't have any legacy pass manager CGSCC passes that modify the call graph (we only use it in the codegen pipeline to run function passes in call graph order). This is the beginning of removing CallGraphUpdater and making all the relevant CGSCC passes directly use the new pass manager APIs.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Arthur Eubanks (aeubanks)

Changes

We don't have any legacy pass manager CGSCC passes that modify the call graph (we only use it in the codegen pipeline to run function passes in call graph order). This is the beginning of removing CallGraphUpdater and making all the relevant CGSCC passes directly use the new pass manager APIs.

---

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

6 Files Affected:

• (modified) llvm/include/llvm/Transforms/IPO/Attributor.h (-8)
• (modified) llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h (-18)
• (modified) llvm/lib/Transforms/IPO/Attributor.cpp (-3)
• (modified) llvm/lib/Transforms/IPO/OpenMPOpt.cpp (-2)
• (modified) llvm/lib/Transforms/Utils/CallGraphUpdater.cpp (+30-101)
• (modified) llvm/unittests/IR/LegacyPassManagerTest.cpp (-132)

diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h
index f70fd02ca573c..34557238ecb23 100644
--- a/llvm/include/llvm/Transforms/IPO/Attributor.h
+++ b/llvm/include/llvm/Transforms/IPO/Attributor.h
@@ -1828,14 +1828,6 @@ struct Attributor {
       Configuration.InitializationCallback(*this, F);
   }
 
-  /// Helper function to remove callsite.
-  void removeCallSite(CallInst *CI) {
-    if (!CI)
-      return;
-
-    Configuration.CGUpdater.removeCallSite(*CI);
-  }
-
   /// Record that \p U is to be replaces with \p NV after information was
   /// manifested. This also triggers deletion of trivially dead istructions.
   bool changeUseAfterManifest(Use &U, Value &NV) {
diff --git a/llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h b/llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h
index 7e6683fd0c8a2..8dc9648059feb 100644
--- a/llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h
+++ b/llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h
@@ -38,12 +38,6 @@ class CallGraphUpdater {
   SmallVector<Function *, 16> DeadFunctionsInComdats;
   ///}
 
-  /// Old PM variables
-  ///{
-  CallGraph *CG = nullptr;
-  CallGraphSCC *CGSCC = nullptr;
-  ///}
-
   /// New PM variables
   ///{
   LazyCallGraph *LCG = nullptr;
@@ -60,10 +54,6 @@ class CallGraphUpdater {
   /// Initializers for usage outside of a CGSCC pass, inside a CGSCC pass in
   /// the old and new pass manager (PM).
   ///{
-  void initialize(CallGraph &CG, CallGraphSCC &SCC) {
-    this->CG = &CG;
-    this->CGSCC = &SCC;
-  }
   void initialize(LazyCallGraph &LCG, LazyCallGraph::SCC &SCC,
                   CGSCCAnalysisManager &AM, CGSCCUpdateResult &UR) {
     this->LCG = &LCG;
@@ -95,14 +85,6 @@ class CallGraphUpdater {
   /// Note that \p OldFn is also removed from the call graph
   /// (\see removeFunction).
   void replaceFunctionWith(Function &OldFn, Function &NewFn);
-
-  /// Remove the call site \p CS from the call graph.
-  void removeCallSite(CallBase &CS);
-
-  /// Replace \p OldCS with the new call site \p NewCS.
-  /// \return True if the replacement was successful, otherwise False. In the
-  /// latter case the parent function of \p OldCB needs to be re-analyzed.
-  bool replaceCallSite(CallBase &OldCS, CallBase &NewCS);
 };
 
 } // end namespace llvm
diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
index 10660b9cb3ca1..915490c922847 100644
--- a/llvm/lib/Transforms/IPO/Attributor.cpp
+++ b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -2557,8 +2557,6 @@ ChangeStatus Attributor::cleanupIR() {
       if (auto *CB = dyn_cast<CallBase>(I)) {
         assert((isa<IntrinsicInst>(CB) || isRunOn(*I->getFunction())) &&
                "Cannot delete an instruction outside the current SCC!");
-        if (!isa<IntrinsicInst>(CB))
-          Configuration.CGUpdater.removeCallSite(*CB);
       }
       I->dropDroppableUses();
       CGModifiedFunctions.insert(I->getFunction());
@@ -3186,7 +3184,6 @@ ChangeStatus Attributor::rewriteFunctionSignatures(
       assert(OldCB.getType() == NewCB.getType() &&
              "Cannot handle call sites with different types!");
       ModifiedFns.insert(OldCB.getFunction());
-      Configuration.CGUpdater.replaceCallSite(OldCB, NewCB);
       OldCB.replaceAllUsesWith(&NewCB);
       OldCB.eraseFromParent();
     }
diff --git a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
index 9e658e91a71a4..b290651d66c58 100644
--- a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
+++ b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
@@ -1453,7 +1453,6 @@ struct OpenMPOpt {
       };
       emitRemark<OptimizationRemark>(CI, "OMP160", Remark);
 
-      CGUpdater.removeCallSite(*CI);
       CI->eraseFromParent();
       Changed = true;
       ++NumOpenMPParallelRegionsDeleted;
@@ -1895,7 +1894,6 @@ struct OpenMPOpt {
       else
         emitRemark<OptimizationRemark>(&F, "OMP170", Remark);
 
-      CGUpdater.removeCallSite(*CI);
       CI->replaceAllUsesWith(ReplVal);
       CI->eraseFromParent();
       ++NumOpenMPRuntimeCallsDeduplicated;
diff --git a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
index 7cba4829d4839..3b6fce578ffcc 100644
--- a/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
+++ b/llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
@@ -13,9 +13,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/CallGraphUpdater.h"
-#include "llvm/ADT/STLExtras.h"
-#include "llvm/Analysis/CallGraph.h"
-#include "llvm/Analysis/CallGraphSCCPass.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
@@ -28,53 +25,33 @@ bool CallGraphUpdater::finalize() {
                          DeadFunctionsInComdats.end());
   }
 
-  if (CG) {
-    // First remove all references, e.g., outgoing via called functions. This is
-    // necessary as we can delete functions that have circular references.
-    for (Function *DeadFn : DeadFunctions) {
-      DeadFn->removeDeadConstantUsers();
-      CallGraphNode *DeadCGN = (*CG)[DeadFn];
-      DeadCGN->removeAllCalledFunctions();
-      CG->getExternalCallingNode()->removeAnyCallEdgeTo(DeadCGN);
-      DeadFn->replaceAllUsesWith(PoisonValue::get(DeadFn->getType()));
-    }
-
-    // Then remove the node and function from the module.
-    for (Function *DeadFn : DeadFunctions) {
-      CallGraphNode *DeadCGN = CG->getOrInsertFunction(DeadFn);
-      assert(DeadCGN->getNumReferences() == 0 &&
-             "References should have been handled by now");
-      delete CG->removeFunctionFromModule(DeadCGN);
-    }
-  } else {
-    // This is the code path for the new lazy call graph and for the case were
-    // no call graph was provided.
-    for (Function *DeadFn : DeadFunctions) {
-      DeadFn->removeDeadConstantUsers();
-      DeadFn->replaceAllUsesWith(PoisonValue::get(DeadFn->getType()));
-
-      if (LCG && !ReplacedFunctions.count(DeadFn)) {
-        // Taken mostly from the inliner:
-        LazyCallGraph::Node &N = LCG->get(*DeadFn);
-        auto *DeadSCC = LCG->lookupSCC(N);
-        assert(DeadSCC && DeadSCC->size() == 1 &&
-               &DeadSCC->begin()->getFunction() == DeadFn);
-
-        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(LCG->lookupSCC(N));
-        UR->DeadFunctions.push_back(DeadFn);
-      } else {
-        // The CGSCC infrastructure batch deletes functions at the end of the
-        // call graph walk, so only erase the function if we're not using that
-        // infrastructure.
-        // The function is now really dead and de-attached from everything.
-        DeadFn->eraseFromParent();
-      }
+  // This is the code path for the new lazy call graph and for the case were
+  // no call graph was provided.
+  for (Function *DeadFn : DeadFunctions) {
+    DeadFn->removeDeadConstantUsers();
+    DeadFn->replaceAllUsesWith(PoisonValue::get(DeadFn->getType()));
+
+    if (LCG && !ReplacedFunctions.count(DeadFn)) {
+      // Taken mostly from the inliner:
+      LazyCallGraph::Node &N = LCG->get(*DeadFn);
+      auto *DeadSCC = LCG->lookupSCC(N);
+      assert(DeadSCC && DeadSCC->size() == 1 &&
+             &DeadSCC->begin()->getFunction() == DeadFn);
+
+      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(LCG->lookupSCC(N));
+      UR->DeadFunctions.push_back(DeadFn);
+    } else {
+      // The CGSCC infrastructure batch deletes functions at the end of the
+      // call graph walk, so only erase the function if we're not using that
+      // infrastructure.
+      // The function is now really dead and de-attached from everything.
+      DeadFn->eraseFromParent();
     }
   }
 
@@ -85,11 +62,7 @@ bool CallGraphUpdater::finalize() {
 }
 
 void CallGraphUpdater::reanalyzeFunction(Function &Fn) {
-  if (CG) {
-    CallGraphNode *OldCGN = CG->getOrInsertFunction(&Fn);
-    OldCGN->removeAllCalledFunctions();
-    CG->populateCallGraphNode(OldCGN);
-  } else if (LCG) {
+  if (LCG) {
     LazyCallGraph::Node &N = LCG->get(Fn);
     LazyCallGraph::SCC *C = LCG->lookupSCC(N);
     updateCGAndAnalysisManagerForCGSCCPass(*LCG, *C, N, *AM, *UR, *FAM);
@@ -98,9 +71,7 @@ void CallGraphUpdater::reanalyzeFunction(Function &Fn) {
 
 void CallGraphUpdater::registerOutlinedFunction(Function &OriginalFn,
                                                 Function &NewFn) {
-  if (CG)
-    CG->addToCallGraph(&NewFn);
-  else if (LCG)
+  if (LCG)
     LCG->addSplitFunction(OriginalFn, NewFn);
 }
 
@@ -112,12 +83,6 @@ void CallGraphUpdater::removeFunction(Function &DeadFn) {
   else
     DeadFunctions.push_back(&DeadFn);
 
-  // For the old call graph we remove the function from the SCC right away.
-  if (CG && !ReplacedFunctions.count(&DeadFn)) {
-    CallGraphNode *DeadCGN = (*CG)[&DeadFn];
-    DeadCGN->removeAllCalledFunctions();
-    CGSCC->DeleteNode(DeadCGN);
-  }
   if (FAM)
     FAM->clear(DeadFn, DeadFn.getName());
 }
@@ -125,46 +90,10 @@ void CallGraphUpdater::removeFunction(Function &DeadFn) {
 void CallGraphUpdater::replaceFunctionWith(Function &OldFn, Function &NewFn) {
   OldFn.removeDeadConstantUsers();
   ReplacedFunctions.insert(&OldFn);
-  if (CG) {
-    // Update the call graph for the newly promoted function.
-    CallGraphNode *OldCGN = (*CG)[&OldFn];
-    CallGraphNode *NewCGN = CG->getOrInsertFunction(&NewFn);
-    NewCGN->stealCalledFunctionsFrom(OldCGN);
-    CG->ReplaceExternalCallEdge(OldCGN, NewCGN);
-
-    // And update the SCC we're iterating as well.
-    CGSCC->ReplaceNode(OldCGN, NewCGN);
-  } else if (LCG) {
+  if (LCG) {
     // Directly substitute the functions in the call graph.
     LazyCallGraph::Node &OldLCGN = LCG->get(OldFn);
     SCC->getOuterRefSCC().replaceNodeFunction(OldLCGN, NewFn);
   }
   removeFunction(OldFn);
 }
-
-bool CallGraphUpdater::replaceCallSite(CallBase &OldCS, CallBase &NewCS) {
-  // This is only necessary in the (old) CG.
-  if (!CG)
-    return true;
-
-  Function *Caller = OldCS.getCaller();
-  CallGraphNode *NewCalleeNode =
-      CG->getOrInsertFunction(NewCS.getCalledFunction());
-  CallGraphNode *CallerNode = (*CG)[Caller];
-  if (llvm::none_of(*CallerNode, [&OldCS](const CallGraphNode::CallRecord &CR) {
-        return CR.first && *CR.first == &OldCS;
-      }))
-    return false;
-  CallerNode->replaceCallEdge(OldCS, NewCS, NewCalleeNode);
-  return true;
-}
-
-void CallGraphUpdater::removeCallSite(CallBase &CS) {
-  // This is only necessary in the (old) CG.
-  if (!CG)
-    return;
-
-  Function *Caller = CS.getCaller();
-  CallGraphNode *CallerNode = (*CG)[Caller];
-  CallerNode->removeCallEdgeFor(CS);
-}
diff --git a/llvm/unittests/IR/LegacyPassManagerTest.cpp b/llvm/unittests/IR/LegacyPassManagerTest.cpp
index 0c8a213df8d94..6a4cc4d7a9829 100644
--- a/llvm/unittests/IR/LegacyPassManagerTest.cpp
+++ b/llvm/unittests/IR/LegacyPassManagerTest.cpp
@@ -565,138 +565,6 @@ namespace llvm {
       return mod;
     }
 
-    /// Split a simple function which contains only a call and a return into two
-    /// such that the first calls the second and the second whoever was called
-    /// initially.
-    Function *splitSimpleFunction(Function &F) {
-      LLVMContext &Context = F.getContext();
-      Function *SF = Function::Create(F.getFunctionType(), F.getLinkage(),
-                                      F.getName() + "b", F.getParent());
-      F.setName(F.getName() + "a");
-      BasicBlock *Entry = BasicBlock::Create(Context, "entry", SF, nullptr);
-      CallInst &CI = cast<CallInst>(F.getEntryBlock().front());
-      CI.clone()->insertBefore(ReturnInst::Create(Context, Entry));
-      CI.setCalledFunction(SF);
-      return SF;
-    }
-
-    struct CGModifierPass : public CGPass {
-      unsigned NumSCCs = 0;
-      unsigned NumFns = 0;
-      unsigned NumFnDecls = 0;
-      unsigned SetupWorked = 0;
-      unsigned NumExtCalledBefore = 0;
-      unsigned NumExtCalledAfter = 0;
-
-      CallGraphUpdater CGU;
-
-      bool runOnSCC(CallGraphSCC &SCMM) override {
-        ++NumSCCs;
-        for (CallGraphNode *N : SCMM) {
-          if (N->getFunction()){
-            ++NumFns;
-            NumFnDecls += N->getFunction()->isDeclaration();
-          }
-        }
-        CGPass::run();
-
-        CallGraph &CG = const_cast<CallGraph &>(SCMM.getCallGraph());
-        CallGraphNode *ExtCallingNode = CG.getExternalCallingNode();
-        NumExtCalledBefore = ExtCallingNode->size();
-
-        if (SCMM.size() <= 1)
-          return false;
-
-        CallGraphNode *N = *(SCMM.begin());
-        Function *F = N->getFunction();
-        Module *M = F->getParent();
-        Function *Test1F = M->getFunction("test1");
-        Function *Test2aF = M->getFunction("test2a");
-        Function *Test2bF = M->getFunction("test2b");
-        Function *Test3F = M->getFunction("test3");
-
-        auto InSCC = [&](Function *Fn) {
-          return llvm::any_of(SCMM, [Fn](CallGraphNode *CGN) {
-            return CGN->getFunction() == Fn;
-          });
-        };
-
-        if (!Test1F || !Test2aF || !Test2bF || !Test3F || !InSCC(Test1F) ||
-            !InSCC(Test2aF) || !InSCC(Test2bF) || !InSCC(Test3F))
-          return false;
-
-        CallInst *CI = dyn_cast<CallInst>(&Test1F->getEntryBlock().front());
-        if (!CI || CI->getCalledFunction() != Test2aF)
-          return false;
-
-        SetupWorked += 1;
-
-        // Create a replica of test3 and just move the blocks there.
-        Function *Test3FRepl = Function::Create(
-            /*Type=*/Test3F->getFunctionType(),
-            /*Linkage=*/GlobalValue::InternalLinkage,
-            /*Name=*/"test3repl", Test3F->getParent());
-        while (!Test3F->empty()) {
-          BasicBlock &BB = Test3F->front();
-          BB.removeFromParent();
-          BB.insertInto(Test3FRepl);
-        }
-
-        CGU.initialize(CG, SCMM);
-
-        // Replace test3 with the replica. This is legal as it is actually
-        // internal and the "capturing use" is not really capturing anything.
-        CGU.replaceFunctionWith(*Test3F, *Test3FRepl);
-        Test3F->replaceAllUsesWith(Test3FRepl);
-
-        // Rewrite the call in test1 to point to the replica of 3 not test2.
-        CI->setCalledFunction(Test3FRepl);
-
-        // Delete test2a and test2b and reanalyze 1 as we changed calls inside.
-        CGU.removeFunction(*Test2aF);
-        CGU.removeFunction(*Test2bF);
-        CGU.reanalyzeFunction(*Test1F);
-
-        return true;
-      }
-
-      bool doFinalization(CallGraph &CG) override {
-        CGU.finalize();
-        // We removed test2 and replaced the internal test3.
-        NumExtCalledAfter = CG.getExternalCallingNode()->size();
-        return true;
-      }
-    };
-
-    TEST(PassManager, CallGraphUpdater0) {
-      // SCC#1: test1->test2a->test2b->test3->test1
-      // SCC#2: test4
-      // SCC#3: test3 (the empty function declaration as we replaced it with
-      //               test3repl when we visited SCC#1)
-      // SCC#4: test2a->test2b (the empty function declarations as we deleted
-      //                        these functions when we visited SCC#1)
-      // SCC#5: indirect call node
-
-      LLVMContext Context;
-      std::unique_ptr<Module> M(makeLLVMModule(Context));
-      ASSERT_EQ(M->getFunctionList().size(), 4U);
-      Function *F = M->getFunction("test2");
-      Function *SF = splitSimpleFunction(*F);
-      CallInst::Create(F, "", &*SF->getEntryBlock().getFirstInsertionPt());
-      ASSERT_EQ(M->getFunctionList().size(), 5U);
-      CGModifierPass *P = new CGModifierPass();
-      legacy::PassManager Passes;
-      Passes.add(P);
-      Passes.run(*M);
-      ASSERT_EQ(P->SetupWorked, 1U);
-      ASSERT_EQ(P->NumSCCs, 4U);
-      ASSERT_EQ(P->NumFns, 6U);
-      ASSERT_EQ(P->NumFnDecls, 1U);
-      ASSERT_EQ(M->getFunctionList().size(), 3U);
-      ASSERT_EQ(P->NumExtCalledBefore, /* test1, 2a, 2b, 3, 4 */ 5U);
-      ASSERT_EQ(P->NumExtCalledAfter, /* test1, 3repl, 4 */ 3U);
-    }
-
     // Test for call graph SCC pass that replaces all callback call instructions
     // with clones and updates CallGraph by calling CallGraph::replaceCallEdge()
     // method. Test is expected to complete successfully after running pass on

[zmodem]

Why are the uses in Attributor and OpenMPOpt safe to remove?

[aeubanks]

the legacy PM version of CallGraphUpdater::initialize() is never called, so all the legacy PM infra in CallGraphUpdater can be deleted. that makes some of the CallGraphUpdater methods empty, so we can delete them and any callers