[test] Cleanup some CGSCCPassManager tests

[aeubanks]

Summary

Don't iterate over SCC as we potentially modify it.
Verify module (and fix some broken ones).
Only run pass once and make sure that it's actually run.
Rename tests to just end in a number since I'm planning on adding a
bunch more which won't have good individual names. Instead, add comments
on the transformations that each test does.

[rnk]

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp

1809

What do you think of this as an alternative code pattern for safe iteration of the nodes?

for (... N : std::vector<LazyCallGraph::Node*>(C.begin(), C.end())) { ... }

[rnk]

lgtm, the comment is a suggestion

[aeubanks]

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp

1809

I initially attempted something like that, the issue is that C.begin()/end() return references and not pointers.

[rnk]

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp

1809

Well, we could make it happen with mapped_iterator, or perhaps just encapsulating the vector construction into a function that returns the right std::vector.

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp

 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Transforms/Utils/CallGraphUpdater.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
CGPM.addPass(LambdaSCCPassNoPreserve(
         ASSERT_NO_FATAL_FAILURE(CGU.removeFunction(*FnH2));
       }));
 
   ModulePassManager MPM(/*DebugLogging*/ true);
   MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
   MPM.run(*M, MAM);
 }
 
-TEST_F(CGSCCPassManagerTest, TestInsertionOfNewRefSCC) {
+// Start with call recursive f, create f -> g and ref recursive f.
+TEST_F(CGSCCPassManagerTest, TestInsertionOfNewFunctions1) {
   std::unique_ptr<Module> M = parseIR("define void @f() {\n"
                                       "entry:\n"
                                       "  call void @f()\n"
                                       "  ret void\n"
                                       "}\n");
 
+  bool Ran = false;
+
   CGSCCPassManager CGPM(/*DebugLogging*/ true);
   CGPM.addPass(LambdaSCCPassNoPreserve(
       [&](LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM, LazyCallGraph &CG,
           CGSCCUpdateResult &UR) {
+        if (Ran)
+          return;
+
         auto &FAM =
             AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, CG).getManager();
 
-        for (auto &N : C) {
-          auto &F = N.getFunction();
+        // Don't iterate over SCC while changing it.
+        SmallVector<LazyCallGraph::Node *> Nodes;
+        for (auto &N : C)
+          Nodes.push_back(&N);
+
+        for (LazyCallGraph::Node *N : Nodes) {
+          Function &F = N->getFunction();
           if (F.getName() != "f")
             continue;
-          auto *Call = dyn_cast<CallInst>(F.begin()->begin());
-          if (!Call || Call->getCalledFunction()->getName() != "f")
-            continue;
 
           // Create a new function 'g'.
           auto *G = Function::Create(F.getFunctionType(), F.getLinkage(),
                                      F.getAddressSpace(), "g", F.getParent());
+          auto *GBB =
               BasicBlock::Create(F.getParent()->getContext(), "entry", G);
+          (void)ReturnInst::Create(G->getContext(), GBB);
           // Instruct the LazyCallGraph to create a new node for 'g', as the
           // single node in a new SCC, into the call graph. As a result
           // the call graph is composed of a single RefSCC with two SCCs:
           // [(f), (g)].
 
-          // "Demote" the 'f -> f' call egde to a ref edge.
+          // "Demote" the 'f -> f' call edge to a ref edge.
           // 1. Erase the call edge from 'f' to 'f'.
-          Call->eraseFromParent();
+          F.getEntryBlock().front().eraseFromParent();
           // 2. Insert a ref edge from 'f' to 'f'.
-          (void)CastInst::CreatePointerCast(&F,
-                                            Type::getInt8PtrTy(F.getContext()),
-                                            "f.ref", &*F.begin()->begin());
+          (void)CastInst::CreatePointerCast(
+              &F, Type::getInt8PtrTy(F.getContext()), "f.ref",
+              &F.getEntryBlock().front());
+
+          ASSERT_FALSE(verifyModule(*F.getParent(), &errs()));
 
           ASSERT_NO_FATAL_FAILURE(
-              updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR, FAM))
+              updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))
               << "Updating the call graph with a demoted, self-referential "
                  "call edge 'f -> f', and a newly inserted ref edge 'f -> g', "
                  "caused a fatal failure";
+
+          Ran = true;
         }
       }));
 
   ModulePassManager MPM(/*DebugLogging*/ true);
   MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
   MPM.run(*M, MAM);
+  ASSERT_TRUE(Ran);
 }
 
-TEST_F(CGSCCPassManagerTest, TestInsertionOfNewRefSCCMutuallyRecursive) {
+// Start with f, end with f -> (g1 <-> g2) and f -ref-> (h1 <-> h2).
+TEST_F(CGSCCPassManagerTest, TestInsertionOfNewFunctions2) {
   std::unique_ptr<Module> M = parseIR("define void @f() {\n"
                                       "entry:\n"
                                       "  ret void\n"
                                       "}\n");
 
+  bool Ran = false;
+
   CGSCCPassManager CGPM(/*DebugLogging*/ true);
   CGPM.addPass(LambdaSCCPassNoPreserve([&](LazyCallGraph::SCC &C,
                                            CGSCCAnalysisManager &AM,
                                            LazyCallGraph &CG,
                                            CGSCCUpdateResult &UR) {
+    if (Ran)
+      return;
+
     auto &FAM =
         AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, CG).getManager();
 
-    for (auto &N : C) {
-      auto &F = N.getFunction();
+    // Don't iterate over SCC while changing it.
+    SmallVector<LazyCallGraph::Node *> Nodes;
+    for (auto &N : C)
+      Nodes.push_back(&N);
+
+    for (LazyCallGraph::Node *N : Nodes) {
+      Function &F = N->getFunction();
       if (F.getName() != "f")
         continue;
 
       // Create mutually recursive functions (call) 'g1' and 'g2'.
       auto *G1 = Function::Create(F.getFunctionType(), F.getLinkage(),
                                   F.getAddressSpace(), "g1", F.getParent());
       auto *G2 = Function::Create(F.getFunctionType(), F.getLinkage(),
                                   F.getAddressSpace(), "g2", F.getParent());
       BasicBlock *G1BB =
           BasicBlock::Create(F.getParent()->getContext(), "entry", G1);
       BasicBlock *G2BB =
           BasicBlock::Create(F.getParent()->getContext(), "entry", G2);
-      (void)CallInst::Create(G1, {}, "", G1BB);
-      (void)ReturnInst::Create(G1->getContext(), G1BB);
       (void)CallInst::Create(G2, {}, "", G1BB);
+      (void)ReturnInst::Create(G1->getContext(), G1BB);
+      (void)CallInst::Create(G1, {}, "", G2BB);
       (void)ReturnInst::Create(G2->getContext(), G2BB);
 
       // Add 'f -> g1' call edge.
       (void)CallInst::Create(G1, {}, "", &F.getEntryBlock().front());
       // Add 'f -> g2' call edge.
       (void)CallInst::Create(G2, {}, "", &F.getEntryBlock().front());
 
       // Create mutually recursive functions (ref only) 'h1' and 'h2'.
for (LazyCallGraph::Node *N : Nodes) {
 
       // Add 'f -> h1' ref edge.
       (void)CastInst::CreatePointerCast(H1, Type::getInt8PtrTy(F.getContext()),
                                         "h1.ref", &F.getEntryBlock().front());
       // Add 'f -> h2' ref edge.
       (void)CastInst::CreatePointerCast(H2, Type::getInt8PtrTy(F.getContext()),
                                         "h2.ref", &F.getEntryBlock().front());
 
+      ASSERT_FALSE(verifyModule(*F.getParent(), &errs()));
+
       ASSERT_NO_FATAL_FAILURE(
-          updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR, FAM))
+          updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))
           << "Updating the call graph with mutually recursive g1 <-> g2, h1 "
              "<-> h2 caused a fatal failure";
+
+      Ran = true;
     }
   }));
 
   ModulePassManager MPM(/*DebugLogging*/ true);
   MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
   MPM.run(*M, MAM);
+  ASSERT_TRUE(Ran);
 }
 
 TEST_F(CGSCCPassManagerTest, TestInsertionOfNewNonTrivialCallEdge) {
   std::unique_ptr<Module> M = parseIR("define void @f1() {\n"
                                       "entry:\n"
                                       "  %a = bitcast void ()* @f4 to i8*\n"
                                       "  %b = bitcast void ()* @f2 to i8*\n"
                                       "  ret void\n"
CGPM.addPass(LambdaSCCPassNoPreserve([&](LazyCallGraph::SCC &C,
                                            LazyCallGraph &CG,
                                            CGSCCUpdateResult &UR) {
     if (Ran)
       return;
 
     auto &FAM =
         AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, CG).getManager();
 
-    for (auto &N : C) {
-      auto &F = N.getFunction();
+    // Don't iterate over SCC while changing it.
+    SmallVector<LazyCallGraph::Node *> Nodes;
+    for (auto &N : C)
+      Nodes.push_back(&N);
+
+    for (LazyCallGraph::Node *N : Nodes) {
+      Function &F = N->getFunction();
       if (F.getName() != "f1")
         continue;
 
       Function *F3 = F.getParent()->getFunction("f3");
       ASSERT_TRUE(F3 != nullptr);
 
       // Create call from f1 to f3.
       (void)CallInst::Create(F3, {}, "", F.getEntryBlock().getTerminator());
 
       ASSERT_NO_FATAL_FAILURE(
-          updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR, FAM))
+          updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))
           << "Updating the call graph with mutually recursive g1 <-> g2, h1 "
              "<-> h2 caused a fatal failure";
 
       Ran = true;
     }
   }));
 
   ModulePassManager MPM(/*DebugLogging*/ true);
   MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
   MPM.run(*M, MAM);
 
   ASSERT_TRUE(Ran);
 }
 
 #endif
 } // namespace