[CallGraphUpdater] Remove nodes from their SCC (old PM)

[jdoerfert]

Summary

We can and should remove deleted nodes from their respective SCCs. We
did not do this before and this was a potential problem even though I
couldn't locally trigger an issue. Since the DeleteNode would assert
if the node was not in the SCC, we know we only remove nodes from their
SCC and only once (when run on all the Attributor tests).

[hfinkel]

LGTM

llvm/lib/Transforms/Utils/CallGraphUpdater.cpp

108

I realize that this probably doesn't matter, but I wonder if the code would be clearer here if you wrote:

if (CG && ...)

because you've dereferencing CG here and use of CGSCC is guarded below by GC (instead of directly by checking CGSCC). Keeping the guarding conditions the same should make the code more readable by eliminating potential questions about why they're different.

[MaskRay]

llvm/lib/Analysis/CallGraphSCCPass.cpp

566

clang-format prefers /*New=*/nullptr

[jdoerfert]

addressed nits

llvm/include/llvm/Analysis/CallGraphSCCPass.h

public:
 
   bool isSingular() const { return Nodes.size() == 1; }
   unsigned size() const { return Nodes.size(); }
 
   /// ReplaceNode - This informs the SCC and the pass manager that the specified
   /// Old node has been deleted, and New is to be used in its place.
   void ReplaceNode(CallGraphNode *Old, CallGraphNode *New);
 
+  /// DeleteNode - This informs the SCC and the pass manager that the specified
+  /// Old node has been deleted.
+  void DeleteNode(CallGraphNode *Old);
+
   using iterator = std::vector<CallGraphNode *>::const_iterator;
 
   iterator begin() const { return Nodes.begin(); }
   iterator end() const { return Nodes.end(); }
 
   const CallGraph &getCallGraph() { return CG; }
 };
 

llvm/lib/Analysis/CallGraphSCCPass.cpp

void CallGraphSCC::ReplaceNode(CallGraphNode *Old, CallGraphNode *New) {
   }
 
   // Update the active scc_iterator so that it doesn't contain dangling
   // pointers to the old CallGraphNode.
   scc_iterator<CallGraph*> *CGI = (scc_iterator<CallGraph*>*)Context;
   CGI->ReplaceNode(Old, New);
 }
 
+void CallGraphSCC::DeleteNode(CallGraphNode *Old) {
+  ReplaceNode(Old, /* New */ nullptr);
+}
+
 //===----------------------------------------------------------------------===//
 // CallGraphSCCPass Implementation
 //===----------------------------------------------------------------------===//
 
 /// Assign pass manager to manage this pass.
 void CallGraphSCCPass::assignPassManager(PMStack &PMS,
                                          PassManagerType PreferredType) {
   // Find CGPassManager

llvm/lib/Transforms/Utils/CallGraphUpdater.cpp

 
 void CallGraphUpdater::removeFunction(Function &DeadFn) {
   DeadFn.deleteBody();
   DeadFn.setLinkage(GlobalValue::ExternalLinkage);
   if (DeadFn.hasComdat())
     DeadFunctionsInComdats.push_back(&DeadFn);
   else
     DeadFunctions.push_back(&DeadFn);
+
+  // For the old call graph we remove the function from the SCC right away.
+  if (CGSCC && !ReplacedFunctions.count(&DeadFn))
+    CGSCC->DeleteNode((*CG)[&DeadFn]);
 }
 
 void CallGraphUpdater::replaceFunctionWith(Function &OldFn, Function &NewFn) {
   OldFn.removeDeadConstantUsers();
   ReplacedFunctions.insert(&OldFn);
   if (CG) {
     // Update the call graph for the newly promoted function.
     // CG->spliceFunction(&OldFn, &NewFn);