diff --git a/include/llvm/Analysis/LazyCallGraph.h b/include/llvm/Analysis/LazyCallGraph.h index 8364465c3dc..566e526f89b 100644 --- a/include/llvm/Analysis/LazyCallGraph.h +++ b/include/llvm/Analysis/LazyCallGraph.h @@ -620,20 +620,32 @@ public: SmallVector switchInternalEdgeToCall(Node &SourceN, Node &TargetN); - /// Make an existing internal call edge into a ref edge. + /// Make an existing internal call edge between separate SCCs into a ref + /// edge. /// - /// If SourceN and TargetN are part of a single SCC, it may be split up due - /// to breaking a cycle in the call edges that formed it. If that happens, - /// then this routine will insert new SCCs into the postorder list *before* - /// the SCC of TargetN (previously the SCC of both). This preserves - /// postorder as the TargetN can reach all of the other nodes by definition - /// of previously being in a single SCC formed by the cycle from SourceN to - /// TargetN. + /// If SourceN and TargetN in separate SCCs within this RefSCC, changing + /// the call edge between them to a ref edge is a trivial operation that + /// does not require any structural changes to the call graph. + void switchTrivialInternalEdgeToRef(Node &SourceN, Node &TargetN); + + /// Make an existing internal call edge within a single SCC into a ref + /// edge. + /// + /// Since SourceN and TargetN are part of a single SCC, this SCC may be + /// split up due to breaking a cycle in the call edges that formed it. If + /// that happens, then this routine will insert new SCCs into the postorder + /// list *before* the SCC of TargetN (previously the SCC of both). This + /// preserves postorder as the TargetN can reach all of the other nodes by + /// definition of previously being in a single SCC formed by the cycle from + /// SourceN to TargetN. /// /// The newly added SCCs are added *immediately* and contiguously /// prior to the TargetN SCC and return the range covering the new SCCs in /// the RefSCC's postorder sequence. You can directly iterate the returned /// range to observe all of the new SCCs in postorder. + /// + /// Note that if SourceN and TargetN are in separate SCCs, the simpler + /// routine `switchTrivialInternalEdgeToRef` should be used instead. iterator_range switchInternalEdgeToRef(Node &SourceN, Node &TargetN); diff --git a/lib/Analysis/CGSCCPassManager.cpp b/lib/Analysis/CGSCCPassManager.cpp index 1d3b184c621..054bdc45ad6 100644 --- a/lib/Analysis/CGSCCPassManager.cpp +++ b/lib/Analysis/CGSCCPassManager.cpp @@ -229,9 +229,7 @@ incorporateNewSCCRange(const SCCRangeT &NewSCCRange, LazyCallGraph &G, if (NewSCCRange.begin() == NewSCCRange.end()) return C; - // Invalidate the analyses of the current SCC and add it to the worklist since - // it has changed its shape. - AM.invalidate(*C, PreservedAnalyses::none()); + // Add the current SCC to the worklist as its shape has changed. UR.CWorklist.insert(C); if (DebugLogging) dbgs() << "Enqueuing the existing SCC in the worklist:" << *C << "\n"; @@ -343,9 +341,24 @@ LazyCallGraph::SCC &llvm::updateCGAndAnalysisManagerForFunctionPass( dbgs() << "Deleting internal " << (IsCall ? "call" : "ref") << " edge from '" << N << "' to '" << TargetN << "'\n"; - if (IsCall) - C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N, TargetN), G, N, - C, AM, UR, DebugLogging); + if (IsCall) { + if (C != &TargetC) { + // For separate SCCs this is trivial. + RC->switchTrivialInternalEdgeToRef(N, TargetN); + } else { + // Otherwise we may end up re-structuring the call graph. First, + // invalidate any SCC analyses. We have to do this before we split + // functions into new SCCs and lose track of where their analyses are + // cached. + // FIXME: We should accept a more precise preserved set here. For + // example, it might be possible to preserve some function analyses + // even as the SCC structure is changed. + AM.invalidate(*C, PreservedAnalyses::none()); + // Now update the call graph. + C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N, TargetN), G, + N, C, AM, UR, DebugLogging); + } + } auto NewRefSCCs = RC->removeInternalRefEdge(N, TargetN); if (!NewRefSCCs.empty()) { @@ -401,10 +414,24 @@ LazyCallGraph::SCC &llvm::updateCGAndAnalysisManagerForFunctionPass( continue; } - // Otherwise we are switching an internal call edge to a ref edge. This - // may split up some SCCs. - C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N, TargetN), G, N, C, - AM, UR, DebugLogging); + // We are switching an internal call edge to a ref edge. This may split up + // some SCCs. + if (C != &TargetC) { + // For separate SCCs this is trivial. + RC->switchTrivialInternalEdgeToRef(N, TargetN); + continue; + } + + // Otherwise we may end up re-structuring the call graph. First, invalidate + // any SCC analyses. We have to do this before we split functions into new + // SCCs and lose track of where their analyses are cached. + // FIXME: We should accept a more precise preserved set here. For example, + // it might be possible to preserve some function analyses even as the SCC + // structure is changed. + AM.invalidate(*C, PreservedAnalyses::none()); + // Now update the call graph. + C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N, TargetN), G, + N, C, AM, UR, DebugLogging); } // Now promote ref edges into call edges. diff --git a/lib/Analysis/LazyCallGraph.cpp b/lib/Analysis/LazyCallGraph.cpp index c08667022b2..f7cf8c6729f 100644 --- a/lib/Analysis/LazyCallGraph.cpp +++ b/lib/Analysis/LazyCallGraph.cpp @@ -606,6 +606,28 @@ LazyCallGraph::RefSCC::switchInternalEdgeToCall(Node &SourceN, Node &TargetN) { return DeletedSCCs; } +void LazyCallGraph::RefSCC::switchTrivialInternalEdgeToRef(Node &SourceN, + Node &TargetN) { + assert(SourceN[TargetN].isCall() && "Must start with a call edge!"); + +#ifndef NDEBUG + // In a debug build, verify the RefSCC is valid to start with and when this + // routine finishes. + verify(); + auto VerifyOnExit = make_scope_exit([&]() { verify(); }); +#endif + + assert(G->lookupRefSCC(SourceN) == this && + "Source must be in this RefSCC."); + assert(G->lookupRefSCC(TargetN) == this && + "Target must be in this RefSCC."); + assert(G->lookupSCC(SourceN) != G->lookupSCC(TargetN) && + "Source and Target must be in separate SCCs for this to be trivial!"); + + // Set the edge kind. + SourceN.setEdgeKind(TargetN.getFunction(), Edge::Ref); +} + iterator_range LazyCallGraph::RefSCC::switchInternalEdgeToRef(Node &SourceN, Node &TargetN) { assert(SourceN[TargetN].isCall() && "Must start with a call edge!"); @@ -617,22 +639,19 @@ LazyCallGraph::RefSCC::switchInternalEdgeToRef(Node &SourceN, Node &TargetN) { auto VerifyOnExit = make_scope_exit([&]() { verify(); }); #endif - SCC &SourceSCC = *G->lookupSCC(SourceN); - SCC &TargetSCC = *G->lookupSCC(TargetN); - - assert(&SourceSCC.getOuterRefSCC() == this && + assert(G->lookupRefSCC(SourceN) == this && "Source must be in this RefSCC."); - assert(&TargetSCC.getOuterRefSCC() == this && + assert(G->lookupRefSCC(TargetN) == this && "Target must be in this RefSCC."); + SCC &TargetSCC = *G->lookupSCC(TargetN); + assert(G->lookupSCC(SourceN) == &TargetSCC && "Source and Target must be in " + "the same SCC to require the " + "full CG update."); + // Set the edge kind. SourceN.setEdgeKind(TargetN.getFunction(), Edge::Ref); - // If this call edge is just connecting two separate SCCs within this RefSCC, - // there is nothing to do. - if (&SourceSCC != &TargetSCC) - return make_range(SCCs.end(), SCCs.end()); - // Otherwise we are removing a call edge from a single SCC. This may break // the cycle. In order to compute the new set of SCCs, we need to do a small // DFS over the nodes within the SCC to form any sub-cycles that remain as diff --git a/test/Transforms/Inline/cgscc-invalidate.ll b/test/Transforms/Inline/cgscc-invalidate.ll new file mode 100644 index 00000000000..60315cda771 --- /dev/null +++ b/test/Transforms/Inline/cgscc-invalidate.ll @@ -0,0 +1,104 @@ +; This test tries to ensure that the inliner successfully invalidates function +; analyses after inlining into the function body. +; +; The strategy for these tests is to compute domtree over all the functions, +; then run the inliner, and then verify the domtree. Then we can arrange the +; inline to disturb the domtree (easy) and detect any stale cached entries in +; the verifier. We do the initial computation both *inside* the CGSCC walk and +; in a pre-step to make sure both work. +; +; RUN: opt < %s -passes='function(require),cgscc(inline,function(verify))' -S | FileCheck %s +; RUN: opt < %s -passes='cgscc(function(require),inline,function(verify))' -S | FileCheck %s + +; An external function used to control branches. +declare i1 @flag() +; CHECK-LABEL: declare i1 @flag() + +; The utility function with interesting control flow that gets inlined below to +; perturb the dominator tree. +define internal void @callee() { +; CHECK-LABEL: @callee +entry: + %ptr = alloca i8 + %flag = call i1 @flag() + br i1 %flag, label %then, label %else + +then: + store volatile i8 42, i8* %ptr + br label %return + +else: + store volatile i8 -42, i8* %ptr + br label %return + +return: + ret void +} + + +; The 'test1_' prefixed functions test the basic scenario of inlining +; destroying dominator tree. + +define void @test1_caller() { +; CHECK-LABEL: define void @test1_caller() +entry: + call void @callee() +; CHECK-NOT: @callee + ret void +; CHECK: ret void +} + + +; The 'test2_' prefixed functions test the scenario of not inlining preserving +; dominators. + +define void @test2_caller() { +; CHECK-LABEL: define void @test2_caller() +entry: + call void @callee() noinline +; CHECK: call void @callee + ret void +; CHECK: ret void +} + + +; The 'test3_' prefixed functions test the scenario of not inlining preserving +; dominators after splitting an SCC into two smaller SCCs. + +; The first function gets visited first and we end up inlining everything we +; can into this routine. That splits test3_g into a separate SCC that is enqued +; for later processing. +define void @test3_f() { +; CHECK-LABEL: define void @test3_f() +entry: + ; Create the first edge in the SCC cycle. + call void @test3_g() +; CHECK-NOT: @test3_g() +; CHECK: call void @test3_f() + + ; Pull interesting CFG into this function. + call void @callee() +; CHECK-NOT: call void @callee() + + ret void +; CHECK: ret void +} + +; This function ends up split into a separate SCC, which can cause its analyses +; to become stale if the splitting doesn't properly invalidate things. Also, as +; a consequence of being split out, test3_f is too large to inline by the time +; we get here. +define void @test3_g() { +; CHECK-LABEL: define void @test3_g() +entry: + ; Create the second edge in the SCC cycle. + call void @test3_f() +; CHECK: call void @test3_f() + + ; Pull interesting CFG into this function. + call void @callee() +; CHECK-NOT: call void @callee() + + ret void +; CHECK: ret void +} diff --git a/unittests/Analysis/LazyCallGraphTest.cpp b/unittests/Analysis/LazyCallGraphTest.cpp index cfbf16c7761..5bb9dec3449 100644 --- a/unittests/Analysis/LazyCallGraphTest.cpp +++ b/unittests/Analysis/LazyCallGraphTest.cpp @@ -1496,7 +1496,7 @@ TEST(LazyCallGraphTest, InternalEdgeMutation) { // Switch the call edge from 'b' to 'c' to a ref edge. This will break the // call cycle and cause us to form more SCCs. The RefSCC will remain the same // though. - RC.switchInternalEdgeToRef(B, C); + auto NewCs = RC.switchInternalEdgeToRef(B, C); EXPECT_EQ(&RC, CG.lookupRefSCC(A)); EXPECT_EQ(&RC, CG.lookupRefSCC(B)); EXPECT_EQ(&RC, CG.lookupRefSCC(C)); @@ -1508,6 +1508,10 @@ TEST(LazyCallGraphTest, InternalEdgeMutation) { EXPECT_EQ(&*J++, CG.lookupSCC(A)); EXPECT_EQ(&*J++, CG.lookupSCC(C)); EXPECT_EQ(RC.end(), J); + // And the returned range must be the slice of this sequence containing new + // SCCs. + EXPECT_EQ(RC.begin(), NewCs.begin()); + EXPECT_EQ(std::prev(RC.end()), NewCs.end()); // Test turning the ref edge from A to C into a call edge. This will form an // SCC out of A and C. Since we previously had a call edge from C to A, the @@ -1710,54 +1714,59 @@ TEST(LazyCallGraphTest, InternalCallEdgeToRef) { EXPECT_EQ(CG.postorder_ref_scc_end(), I); EXPECT_EQ(1, RC.size()); - LazyCallGraph::SCC &CallC = *RC.begin(); + LazyCallGraph::SCC &AC = *RC.begin(); - LazyCallGraph::Node &A = *CG.lookup(lookupFunction(*M, "a")); - LazyCallGraph::Node &B = *CG.lookup(lookupFunction(*M, "b")); - LazyCallGraph::Node &C = *CG.lookup(lookupFunction(*M, "c")); - EXPECT_EQ(&CallC, CG.lookupSCC(A)); - EXPECT_EQ(&CallC, CG.lookupSCC(B)); - EXPECT_EQ(&CallC, CG.lookupSCC(C)); + LazyCallGraph::Node &AN = *CG.lookup(lookupFunction(*M, "a")); + LazyCallGraph::Node &BN = *CG.lookup(lookupFunction(*M, "b")); + LazyCallGraph::Node &CN = *CG.lookup(lookupFunction(*M, "c")); + EXPECT_EQ(&AC, CG.lookupSCC(AN)); + EXPECT_EQ(&AC, CG.lookupSCC(BN)); + EXPECT_EQ(&AC, CG.lookupSCC(CN)); // Remove the call edge from b -> a to a ref edge, which should leave the // 3 functions still in a single connected component because of a -> b -> // c -> a. - RC.switchInternalEdgeToRef(B, A); + auto NewCs = RC.switchInternalEdgeToRef(BN, AN); + EXPECT_EQ(NewCs.begin(), NewCs.end()); EXPECT_EQ(1, RC.size()); - EXPECT_EQ(&CallC, CG.lookupSCC(A)); - EXPECT_EQ(&CallC, CG.lookupSCC(B)); - EXPECT_EQ(&CallC, CG.lookupSCC(C)); + EXPECT_EQ(&AC, CG.lookupSCC(AN)); + EXPECT_EQ(&AC, CG.lookupSCC(BN)); + EXPECT_EQ(&AC, CG.lookupSCC(CN)); // Remove the edge from c -> a, which should leave 'a' in the original SCC // and form a new SCC for 'b' and 'c'. - RC.switchInternalEdgeToRef(C, A); + NewCs = RC.switchInternalEdgeToRef(CN, AN); + EXPECT_EQ(1, std::distance(NewCs.begin(), NewCs.end())); EXPECT_EQ(2, RC.size()); - EXPECT_EQ(&CallC, CG.lookupSCC(A)); - LazyCallGraph::SCC &BCallC = *CG.lookupSCC(B); - EXPECT_NE(&BCallC, &CallC); - EXPECT_EQ(&BCallC, CG.lookupSCC(C)); - auto J = RC.find(CallC); - EXPECT_EQ(&CallC, &*J); + EXPECT_EQ(&AC, CG.lookupSCC(AN)); + LazyCallGraph::SCC &BC = *CG.lookupSCC(BN); + EXPECT_NE(&BC, &AC); + EXPECT_EQ(&BC, CG.lookupSCC(CN)); + auto J = RC.find(AC); + EXPECT_EQ(&AC, &*J); --J; - EXPECT_EQ(&BCallC, &*J); + EXPECT_EQ(&BC, &*J); EXPECT_EQ(RC.begin(), J); + EXPECT_EQ(J, NewCs.begin()); // Remove the edge from c -> b, which should leave 'b' in the original SCC // and form a new SCC for 'c'. It shouldn't change 'a's SCC. - RC.switchInternalEdgeToRef(C, B); + NewCs = RC.switchInternalEdgeToRef(CN, BN); + EXPECT_EQ(1, std::distance(NewCs.begin(), NewCs.end())); EXPECT_EQ(3, RC.size()); - EXPECT_EQ(&CallC, CG.lookupSCC(A)); - EXPECT_EQ(&BCallC, CG.lookupSCC(B)); - LazyCallGraph::SCC &CCallC = *CG.lookupSCC(C); - EXPECT_NE(&CCallC, &CallC); - EXPECT_NE(&CCallC, &BCallC); - J = RC.find(CallC); - EXPECT_EQ(&CallC, &*J); + EXPECT_EQ(&AC, CG.lookupSCC(AN)); + EXPECT_EQ(&BC, CG.lookupSCC(BN)); + LazyCallGraph::SCC &CC = *CG.lookupSCC(CN); + EXPECT_NE(&CC, &AC); + EXPECT_NE(&CC, &BC); + J = RC.find(AC); + EXPECT_EQ(&AC, &*J); --J; - EXPECT_EQ(&BCallC, &*J); + EXPECT_EQ(&BC, &*J); --J; - EXPECT_EQ(&CCallC, &*J); + EXPECT_EQ(&CC, &*J); EXPECT_EQ(RC.begin(), J); + EXPECT_EQ(J, NewCs.begin()); } TEST(LazyCallGraphTest, InternalRefEdgeToCall) { @@ -1927,11 +1936,11 @@ TEST(LazyCallGraphTest, InternalRefEdgeToCallNoCycleInterleaved) { // Several call edges are initially present to force a particual post-order. // Remove them now, leaving an interleaved post-order pattern. - RC.switchInternalEdgeToRef(B3, C3); - RC.switchInternalEdgeToRef(C2, B3); - RC.switchInternalEdgeToRef(B2, C2); - RC.switchInternalEdgeToRef(C1, B2); - RC.switchInternalEdgeToRef(B1, C1); + RC.switchTrivialInternalEdgeToRef(B3, C3); + RC.switchTrivialInternalEdgeToRef(C2, B3); + RC.switchTrivialInternalEdgeToRef(B2, C2); + RC.switchTrivialInternalEdgeToRef(C1, B2); + RC.switchTrivialInternalEdgeToRef(B1, C1); // Check the initial post-order. We ensure this order with the extra edges // that are nuked above. @@ -2054,8 +2063,8 @@ TEST(LazyCallGraphTest, InternalRefEdgeToCallBothPartitionAndMerge) { LazyCallGraph::SCC &GC = *CG.lookupSCC(G); // Remove the extra edges that were used to force a particular post-order. - RC.switchInternalEdgeToRef(C, D); - RC.switchInternalEdgeToRef(D, E); + RC.switchTrivialInternalEdgeToRef(C, D); + RC.switchTrivialInternalEdgeToRef(D, E); // Check the initial post-order. We ensure this order with the extra edges // that are nuked above.