[PM] Teach the CGSCC's CG update utility to more carefully invalidate

analyses when we're about to break apart an SCC.

We can't wait until after breaking apart the SCC to invalidate things:
1) Which SCC do we then invalidate? All of them?
2) Even if we invalidate all of them, a newly created SCC may not have
   a proxy that will convey the invalidation to functions!

Previously we only invalidated one of the SCCs and too late. This led to
stale analyses remaining in the cache. And because the caching strategy
actually works, they would get used and chaos would ensue.

Doing invalidation early is somewhat pessimizing though if we *know*
that the SCC structure won't change. So it turns out that the design to
make the mutation API force the caller to know the *kind* of mutation in
advance was indeed 100% correct and we didn't do enough of it. So this
change also splits two cases of switching a call edge to a ref edge into
two separate APIs so that callers can clearly test for this and take the
easy path without invalidating when appropriate. This is particularly
important in this case as we expect most inlines to be between functions
in separate SCCs and so the common case is that we don't have to so
aggressively invalidate analyses.

The LCG API change in turn needed some basic cleanups and better testing
in its unittest. No interesting functionality changed there other than
more coverage of the returned sequence of SCCs.

While this seems like an obvious improvement over the current state, I'd
like to revisit the core concept of invalidating within the CG-update
layer at all. I'm wondering if we would be better served forcing the
callers to handle the invalidation beforehand in the cases that they
can handle it. An interesting example is when we want to teach the
inliner to *update and preserve* analyses. But we can cross that bridge
when we get there.

With this patch, the new pass manager an build all of the LLVM test
suite at -O3 and everything passes. =D I haven't bootstrapped yet and
I'm sure there are still plenty of bugs, but this gives a nice baseline
so I'm going to increasingly focus on fleshing out the missing
functionality, especially the bits that are just turned off right now in
order to let us establish this baseline.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@290664 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/Analysis/LazyCallGraph.h

@@ -620,20 +620,32 @@ public:
     SmallVector<SCC *, 1> 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<iterator> switchInternalEdgeToRef(Node &SourceN,
                                                      Node &TargetN);
 

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.

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::iterator>
 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

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<domtree>),cgscc(inline,function(verify<domtree>))' -S | FileCheck %s
+; RUN: opt < %s -passes='cgscc(function(require<domtree>),inline,function(verify<domtree>))' -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
+}

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.