From a541b0fde2ab6b8b037edf113d42da41a2c5aae9 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Tue, 1 Sep 2009 06:31:31 +0000 Subject: [PATCH] Change CallGraphNode to maintain it's Function as an AssertingVH for sanity. This didn't turn up any bugs. Change CallGraphNode to maintain its "callsite" information in the call edges list as a WeakVH instead of as an instruction*. This fixes a broad class of dangling pointer bugs, and makes CallGraph have a number of useful invariants again. This fixes the class of problem indicated by PR4029 and PR3601. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@80663 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Analysis/CallGraph.h | 27 +++++++---- lib/Analysis/IPA/CallGraph.cpp | 23 ++------- lib/Analysis/IPA/CallGraphSCCPass.cpp | 48 ++++++++++++------- lib/Transforms/IPO/PruneEH.cpp | 10 ++-- lib/Transforms/Utils/InlineFunction.cpp | 2 +- .../ArgumentPromotion/callgraph-update.ll | 23 +++++++++ 6 files changed, 79 insertions(+), 54 deletions(-) create mode 100644 test/Transforms/ArgumentPromotion/callgraph-update.ll diff --git a/include/llvm/Analysis/CallGraph.h b/include/llvm/Analysis/CallGraph.h index 0e997cab7ff..ff9f1196ae7 100644 --- a/include/llvm/Analysis/CallGraph.h +++ b/include/llvm/Analysis/CallGraph.h @@ -55,6 +55,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/Pass.h" #include "llvm/Support/CallSite.h" +#include "llvm/Support/ValueHandle.h" #include "llvm/System/IncludeFile.h" #include @@ -158,11 +159,16 @@ protected: }; //===----------------------------------------------------------------------===// -// CallGraphNode class definition +// CallGraphNode class definition. // class CallGraphNode { - Function *F; - typedef std::pair CallRecord; + AssertingVH F; + + // CallRecord - This is a pair of the calling instruction (a call or invoke) + // and the callgraph node being called. +public: + typedef std::pair CallRecord; +private: std::vector CalledFunctions; /// NumReferences - This is the number of times that this CallGraphNode occurs @@ -240,19 +246,22 @@ public: /// addCalledFunction - Add a function to the list of functions called by this /// one. void addCalledFunction(CallSite CS, CallGraphNode *M) { - CalledFunctions.push_back(std::make_pair(CS, M)); + CalledFunctions.push_back(std::make_pair(CS.getInstruction(), M)); M->AddRef(); } + void removeCallEdge(iterator I) { + I->second->DropRef(); + *I = CalledFunctions.back(); + CalledFunctions.pop_back(); + } + + /// removeCallEdgeFor - This method removes the edge in the node for the /// specified call site. Note that this method takes linear time, so it /// should be used sparingly. void removeCallEdgeFor(CallSite CS); - // FIXME: REMOVE THIS WHEN HACK IS REMOVED FROM CGSCCPASSMGR. - void removeCallEdgeFor(Instruction *CS); - - /// removeAnyCallEdgeTo - This method removes all call edges from this node /// to the specified callee function. This takes more time to execute than /// removeCallEdgeTo, so it should not be used unless necessary. @@ -278,7 +287,7 @@ public: template <> struct GraphTraits { typedef CallGraphNode NodeType; - typedef std::pair CGNPairTy; + typedef CallGraphNode::CallRecord CGNPairTy; typedef std::pointer_to_unary_function CGNDerefFun; static NodeType *getEntryNode(CallGraphNode *CGN) { return CGN; } diff --git a/lib/Analysis/IPA/CallGraph.cpp b/lib/Analysis/IPA/CallGraph.cpp index 5757bfd897f..d70b7ab7631 100644 --- a/lib/Analysis/IPA/CallGraph.cpp +++ b/lib/Analysis/IPA/CallGraph.cpp @@ -241,7 +241,7 @@ void CallGraphNode::dump() const { print(errs()); } void CallGraphNode::removeCallEdgeFor(CallSite CS) { for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) { assert(I != CalledFunctions.end() && "Cannot find callsite to remove!"); - if (I->first == CS) { + if (I->first == CS.getInstruction()) { I->second->DropRef(); *I = CalledFunctions.back(); CalledFunctions.pop_back(); @@ -250,21 +250,6 @@ void CallGraphNode::removeCallEdgeFor(CallSite CS) { } } -// FIXME: REMOVE THIS WHEN HACK IS REMOVED FROM CGSCCPASSMGR. -void CallGraphNode::removeCallEdgeFor(Instruction *CS) { - for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) { - assert(I != CalledFunctions.end() && "Cannot find callsite to remove!"); - if (I->first.getInstruction() == CS) { - I->second->DropRef(); - *I = CalledFunctions.back(); - CalledFunctions.pop_back(); - return; - } - } - -} - - // removeAnyCallEdgeTo - This method removes any call edges from this node to // the specified callee function. This takes more time to execute than @@ -285,7 +270,7 @@ void CallGraphNode::removeOneAbstractEdgeTo(CallGraphNode *Callee) { for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) { assert(I != CalledFunctions.end() && "Cannot find callee to remove!"); CallRecord &CR = *I; - if (CR.second == Callee && CR.first.getInstruction() == 0) { + if (CR.second == Callee && CR.first == 0) { Callee->DropRef(); *I = CalledFunctions.back(); CalledFunctions.pop_back(); @@ -301,9 +286,9 @@ void CallGraphNode::replaceCallSite(CallSite Old, CallSite New, CallGraphNode *NewCallee) { for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) { assert(I != CalledFunctions.end() && "Cannot find callsite to replace!"); - if (I->first != Old) continue; + if (I->first != Old.getInstruction()) continue; - I->first = New; + I->first = New.getInstruction(); // If the callee is changing, not just the callsite, then update it as // well. diff --git a/lib/Analysis/IPA/CallGraphSCCPass.cpp b/lib/Analysis/IPA/CallGraphSCCPass.cpp index 2d5600d6e79..1dec9c5b6b3 100644 --- a/lib/Analysis/IPA/CallGraphSCCPass.cpp +++ b/lib/Analysis/IPA/CallGraphSCCPass.cpp @@ -125,7 +125,7 @@ bool CGPassManager::RunPassOnSCC(Pass *P, std::vector &CurSCC, void CGPassManager::RefreshCallGraph(std::vector &CurSCC, CallGraph &CG) { - DenseMap CallSites; + DenseMap CallSites; DEBUG(errs() << "CGSCCPASSMGR: Refreshing SCC with " << CurSCC.size() << " nodes:\n"; @@ -146,12 +146,28 @@ void CGPassManager::RefreshCallGraph(std::vector &CurSCC, // Get the set of call sites currently in the function. for (CallGraphNode::iterator I = CGN->begin(), E = CGN->end(); I != E; ++I){ - assert(I->first.getInstruction() && - "Call site record in function should not be abstract"); - assert(!CallSites.count(I->first.getInstruction()) && + // If this call site is null, then the function pass deleted the call + // entirely and the WeakVH nulled it out. + if (I->first == 0 || + // If we've already seen this call site, then the FunctionPass RAUW'd + // one call with another, which resulted in two "uses" in the edge + // list of the same call. + CallSites.count(I->first) || + + // If the call edge is not from a call or invoke, then the function + // pass RAUW'd a call with another value. This can happen when + // constant folding happens of well known functions etc. + CallSite::get(I->first).getInstruction() == 0) { + // Just remove the edge from the set of callees. + CGN->removeCallEdge(I); + E = CGN->end(); + --I; + continue; + } + + assert(!CallSites.count(I->first) && "Call site occurs in node multiple times"); - CallSites.insert(std::make_pair(I->first.getInstruction(), - I->second)); + CallSites.insert(std::make_pair(I->first, I->second)); } // Loop over all of the instructions in the function, getting the callsites. @@ -162,7 +178,7 @@ void CGPassManager::RefreshCallGraph(std::vector &CurSCC, // If this call site already existed in the callgraph, just verify it // matches up to expectations and remove it from CallSites. - DenseMap::iterator ExistingIt = + DenseMap::iterator ExistingIt = CallSites.find(CS.getInstruction()); if (ExistingIt != CallSites.end()) { CallGraphNode *ExistingNode = ExistingIt->second; @@ -201,18 +217,14 @@ void CGPassManager::RefreshCallGraph(std::vector &CurSCC, } // After scanning this function, if we still have entries in callsites, then - // they are dangling pointers. Crap. Well, until we change CallGraph to - // use CallbackVH, we'll just zap them here. When we have that, this should - // turn into an assertion. - if (CallSites.empty()) continue; + // they are dangling pointers. WeakVH should save us for this, so abort if + // this happens. + assert(CallSites.empty() && "Dangling pointers found in call sites map"); - for (DenseMap::iterator I = CallSites.begin(), - E = CallSites.end(); I != E; ++I) - // FIXME: I had to add a special horrible form of removeCallEdgeFor to - // support this. Remove the Instruction* version of it when we can. - CGN->removeCallEdgeFor(I->first); - MadeChange = true; - CallSites.clear(); + // Periodically do an explicit clear to remove tombstones when processing + // large scc's. + if ((sccidx & 15) == 0) + CallSites.clear(); } DEBUG(if (MadeChange) { diff --git a/lib/Transforms/IPO/PruneEH.cpp b/lib/Transforms/IPO/PruneEH.cpp index d9b867e828b..daf81e9259d 100644 --- a/lib/Transforms/IPO/PruneEH.cpp +++ b/lib/Transforms/IPO/PruneEH.cpp @@ -165,9 +165,6 @@ bool PruneEH::runOnSCC(std::vector &SCC) { // function if we have invokes to non-unwinding functions or code after calls to // no-return functions. bool PruneEH::SimplifyFunction(Function *F) { - CallGraph &CG = getAnalysis(); - CallGraphNode *CGN = CG[F]; - bool MadeChange = false; for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) { if (InvokeInst *II = dyn_cast(BB->getTerminator())) @@ -181,14 +178,13 @@ bool PruneEH::SimplifyFunction(Function *F) { Call->setAttributes(II->getAttributes()); // Anything that used the value produced by the invoke instruction - // now uses the value produced by the call instruction. + // now uses the value produced by the call instruction. Note that we + // do this even for void functions and calls with no uses so that the + // callgraph edge is updated. II->replaceAllUsesWith(Call); BasicBlock *UnwindBlock = II->getUnwindDest(); UnwindBlock->removePredecessor(II->getParent()); - // Fix up the call graph. - CGN->replaceCallSite(II, Call, 0/*keep callee*/); - // Insert a branch to the normal destination right before the // invoke. BranchInst::Create(II->getNormalDest(), II); diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp index 4e738e498b3..00ccaf8fcff 100644 --- a/lib/Transforms/Utils/InlineFunction.cpp +++ b/lib/Transforms/Utils/InlineFunction.cpp @@ -212,7 +212,7 @@ static void UpdateCallGraphAfterInlining(CallSite CS, } for (; I != E; ++I) { - const Instruction *OrigCall = I->first.getInstruction(); + const Value *OrigCall = I->first; DenseMap::iterator VMI = ValueMap.find(OrigCall); // Only copy the edge if the call was inlined! diff --git a/test/Transforms/ArgumentPromotion/callgraph-update.ll b/test/Transforms/ArgumentPromotion/callgraph-update.ll new file mode 100644 index 00000000000..2c854bbda98 --- /dev/null +++ b/test/Transforms/ArgumentPromotion/callgraph-update.ll @@ -0,0 +1,23 @@ +; RUN: llvm-as < %s | opt -argpromotion -simplifycfg -constmerge | llvm-dis +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128" +target triple = "i386-apple-darwin10.0" + +%struct.VEC2 = type { double, double, double } +%struct.VERTEX = type { %struct.VEC2, %struct.VERTEX*, %struct.VERTEX* } +%struct.edge_rec = type { %struct.VERTEX*, %struct.edge_rec*, i32, i8* } + +declare %struct.edge_rec* @alloc_edge() nounwind ssp + +define i64 @build_delaunay(%struct.VERTEX* %tree, %struct.VERTEX* %extra) nounwind ssp { +entry: + br i1 undef, label %bb11, label %bb12 + +bb11: ; preds = %bb10 + %a = call %struct.edge_rec* @alloc_edge() nounwind ; <%struct.edge_rec*> [#uses=0] + ret i64 123 + +bb12: ; preds = %bb10 + %b = call %struct.edge_rec* @alloc_edge() nounwind ; <%struct.edge_rec*> [#uses=1] + %c = ptrtoint %struct.edge_rec* %b to i64 + ret i64 %c +}