<div dir="ltr">Thanks for the review. I just wanted to mention I saw it and am working on it. This is mostly cleanup, so it may not be at the top of the queue, but I really don't want to forget it either...</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 23, 2016 at 8:33 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 23, 2016 at 2:02 AM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Tue Feb 23 04:02:02 2016<br>
New Revision: 261624<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=261624&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=261624&view=rev</a><br>
Log:<br>
[PM] Add a unittest for the CGSCC pass manager in the new pass manager<br>
system.<br>
<br>
Previously, this was only being tested with larger integration tests.<br>
That makes it hard to isolated specific issues with it, and makes the<br>
APIs themselves less well tested. Add a unittest based around the same<br>
patterns used for testing the general pass manager.<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>*thumbs up* for unit testing this sort of stuff - thanks for adding it!<br> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Added:<br>
    llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp<br>
Modified:<br>
    llvm/trunk/unittests/Analysis/CMakeLists.txt<br>
<br>
Added: llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp?rev=261624&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp?rev=261624&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp (added)<br>
+++ llvm/trunk/unittests/Analysis/CGSCCPassManagerTest.cpp Tue Feb 23 04:02:02 2016<br>
@@ -0,0 +1,287 @@<br>
+//===- CGSCCPassManagerTest.cpp -------------------------------------------===//<br>
+//<br>
+//                     The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "llvm/Analysis/CGSCCPassManager.h"<br>
+#include "llvm/Analysis/LazyCallGraph.h"<br>
+#include "llvm/AsmParser/Parser.h"<br>
+#include "llvm/IR/Function.h"<br>
+#include "llvm/IR/InstIterator.h"<br>
+#include "llvm/IR/LLVMContext.h"<br>
+#include "llvm/IR/Module.h"<br>
+#include "llvm/IR/PassManager.h"<br>
+#include "llvm/Support/SourceMgr.h"<br>
+#include "gtest/gtest.h"<br>
+<br>
+using namespace llvm;<br>
+<br>
+namespace {<br>
+<br>
+class TestModuleAnalysis {<br>
+public:<br>
+  struct Result {<br>
+    Result(int Count) : FunctionCount(Count) {}<br>
+    int FunctionCount;<br>
+  };<br>
+<br>
+  static void *ID() { return (void *)&PassID; }<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Just a thought - could we remove the burden for passes to provide their own ID by using a template helper? (template<typename Pass> struct PassIdentifier { static const char ID; }; sort of thing) or is it helpful to have implementations be able to differ here? (or just to have simpler API/interaction?)</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  static StringRef name() { return "TestModuleAnalysis"; }<br>
+<br>
+  TestModuleAnalysis(int &Runs) : Runs(Runs) {}<br>
+<br>
+  Result run(Module &M, ModuleAnalysisManager *AM) {<br>
+    ++Runs;<br>
+    return Result(M.size());<br>
+  }<br>
+<br>
+private:<br>
+  static char PassID;<br>
+<br>
+  int &Runs;<br>
+};<br>
+<br>
+char TestModuleAnalysis::PassID;<br>
+<br>
+class TestSCCAnalysis {<br>
+public:<br>
+  struct Result {<br>
+    Result(int Count) : FunctionCount(Count) {}<br>
+    int FunctionCount;<br>
+  };<br>
+<br>
+  static void *ID() { return (void *)&PassID; }<br>
+  static StringRef name() { return "TestSCCAnalysis"; }<br>
+<br>
+  TestSCCAnalysis(int &Runs) : Runs(Runs) {}<br>
+<br>
+  Result run(LazyCallGraph::SCC &C, CGSCCAnalysisManager *AM) {<br>
+    ++Runs;<br>
+    return Result(C.size());<br>
+  }<br>
+<br>
+private:<br>
+  static char PassID;<br>
+<br>
+  int &Runs;<br>
+};<br>
+<br>
+char TestSCCAnalysis::PassID;<br>
+<br>
+class TestFunctionAnalysis {<br>
+public:<br>
+  struct Result {<br>
+    Result(int Count) : InstructionCount(Count) {}<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I think most, if not all, the ctors in here could be omitted & {} init used instead. At least for these Result structs that seems OK? Maybe even for the passes - which just init their members in turn anyway (though I'm not sure off the top of my head how {} init would interact with reference members)</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    int InstructionCount;<br>
+  };<br>
+<br>
+  static void *ID() { return (void *)&PassID; }<br>
+  static StringRef name() { return "TestFunctionAnalysis"; }<br>
+<br>
+  TestFunctionAnalysis(int &Runs) : Runs(Runs) {}<br>
+<br>
+  Result run(Function &F, FunctionAnalysisManager *AM) {<br>
+    ++Runs;<br>
+    int Count = 0;<br>
+    for (Instruction &I : instructions(F)) {<br>
+      (void)I;<br>
+      ++Count;<br>
+    }<br>
+    return Result(Count);<br>
+  }<br>
+<br>
+private:<br>
+  static char PassID;<br>
+<br>
+  int &Runs;<br>
+};<br>
+<br>
+char TestFunctionAnalysis::PassID;<br>
+<br>
+struct TestModulePass {<br>
+  TestModulePass(int &RunCount) : RunCount(RunCount) {}<br>
+<br>
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager *AM) {<br>
+    ++RunCount;<br>
+    (void)AM->getResult<TestModuleAnalysis>(M);<br>
+    return PreservedAnalyses::all();<br>
+  }<br>
+<br>
+  static StringRef name() { return "TestModulePass"; }<br>
+<br>
+  int &RunCount;<br>
+};<br>
+<br>
+struct TestSCCPass {<br>
+  TestSCCPass(int &RunCount, int &AnalyzedInstrCount,<br>
+                   int &AnalyzedSCCFunctionCount,<br>
+                   int &AnalyzedModuleFunctionCount,<br>
+                   bool OnlyUseCachedResults = false)<br>
+      : RunCount(RunCount), AnalyzedInstrCount(AnalyzedInstrCount),<br>
+        AnalyzedSCCFunctionCount(AnalyzedSCCFunctionCount),<br>
+        AnalyzedModuleFunctionCount(AnalyzedModuleFunctionCount),<br>
+        OnlyUseCachedResults(OnlyUseCachedResults) {}<br>
+<br>
+  PreservedAnalyses run(LazyCallGraph::SCC &C, CGSCCAnalysisManager *AM) {<br>
+    ++RunCount;<br>
+<br>
+    const ModuleAnalysisManager &MAM =<br>
+        AM->getResult<ModuleAnalysisManagerCGSCCProxy>(C).getManager();<br>
+    FunctionAnalysisManager &FAM =<br>
+        AM->getResult<FunctionAnalysisManagerCGSCCProxy>(C).getManager();<br>
+    if (TestModuleAnalysis::Result *TMA =<br>
+            MAM.getCachedResult<TestModuleAnalysis>(<br>
+                *C.begin()->getFunction().getParent()))<br>
+      AnalyzedModuleFunctionCount += TMA->FunctionCount;<br>
+<br>
+    if (OnlyUseCachedResults) {<br>
+      // Hack to force the use of the cached interface.<br>
+      if (TestSCCAnalysis::Result *AR =<br>
+              AM->getCachedResult<TestSCCAnalysis>(C))<br>
+        AnalyzedSCCFunctionCount += AR->FunctionCount;<br>
+      for (LazyCallGraph::Node &N : C)<br>
+        if (TestFunctionAnalysis::Result *FAR =<br>
+                FAM.getCachedResult<TestFunctionAnalysis>(N.getFunction()))<br>
+          AnalyzedInstrCount += FAR->InstructionCount;<br>
+    } else {<br>
+      // Typical path just runs the analysis as needed.<br>
+      TestSCCAnalysis::Result &AR = AM->getResult<TestSCCAnalysis>(C);<br>
+      AnalyzedSCCFunctionCount += AR.FunctionCount;<br>
+      for (LazyCallGraph::Node &N : C) {<br>
+        TestFunctionAnalysis::Result &FAR =<br>
+            FAM.getResult<TestFunctionAnalysis>(N.getFunction());<br>
+        AnalyzedInstrCount += FAR.InstructionCount;<br>
+      }<br>
+    }<br>
+<br>
+    return PreservedAnalyses::all();<br>
+  }<br>
+<br>
+  static StringRef name() { return "TestSCCPass"; }<br>
+<br>
+  int &RunCount;<br>
+  int &AnalyzedInstrCount;<br>
+  int &AnalyzedSCCFunctionCount;<br>
+  int &AnalyzedModuleFunctionCount;<br>
+  bool OnlyUseCachedResults;<br>
+};<br>
+<br>
+struct TestFunctionPass {<br>
+  TestFunctionPass(int &RunCount) : RunCount(RunCount) {}<br>
+<br>
+  PreservedAnalyses run(Function &M) {<br>
+    ++RunCount;<br>
+    return PreservedAnalyses::none();<br>
+  }<br>
+<br>
+  static StringRef name() { return "TestFunctionPass"; }<br>
+<br>
+  int &RunCount;<br>
+};<br>
+<br>
+std::unique_ptr<Module> parseIR(const char *IR) {<br>
+  LLVMContext &C = getGlobalContext();<br>
+  SMDiagnostic Err;<br>
+  return parseAssemblyString(IR, Err, C);<br>
+}<br>
+<br>
+class CGSCCPassManagerTest : public ::testing::Test {<br>
+protected:<br>
+  std::unique_ptr<Module> M;<br>
+<br>
+public:<br>
+  CGSCCPassManagerTest()<br>
+      : M(parseIR("define void @f() {\n"<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Can/should we use the fancy C++11 string literals here?</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+                  "entry:\n"<br>
+                  "  call void @g()\n"<br>
+                  "  call void @h1()\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  "define void @g() {\n"<br>
+                  "entry:\n"<br>
+                  "  call void @g()\n"<br>
+                  "  call void @x()\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  "define void @h1() {\n"<br>
+                  "entry:\n"<br>
+                  "  call void @h2()\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  "define void @h2() {\n"<br>
+                  "entry:\n"<br>
+                  "  call void @h3()\n"<br>
+                  "  call void @x()\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  "define void @h3() {\n"<br>
+                  "entry:\n"<br>
+                  "  call void @h1()\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  "define void @x() {\n"<br>
+                  "entry:\n"<br>
+                  "  ret void\n"<br>
+                  "}\n"<br>
+                  )) {}<br>
+};<br>
+<br>
+TEST_F(CGSCCPassManagerTest, Basic) {<br>
+  FunctionAnalysisManager FAM(/*DebugLogging*/ true);<br>
+  int FunctionAnalysisRuns = 0;<br>
+  FAM.registerPass([&] { return TestFunctionAnalysis(FunctionAnalysisRuns); });<br>
+<br>
+  CGSCCAnalysisManager CGAM(/*DebugLogging*/ true);<br>
+  int SCCAnalysisRuns = 0;<br>
+  CGAM.registerPass([&] { return TestSCCAnalysis(SCCAnalysisRuns); });<br>
+<br>
+  ModuleAnalysisManager MAM(/*DebugLogging*/ true);<br>
+  int ModuleAnalysisRuns = 0;<br>
+  MAM.registerPass([&] { return LazyCallGraphAnalysis(); });<br>
+  MAM.registerPass([&] { return TestModuleAnalysis(ModuleAnalysisRuns); });<br>
+<br>
+  MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });<br>
+  MAM.registerPass([&] { return CGSCCAnalysisManagerModuleProxy(CGAM); });<br>
+  CGAM.registerPass([&] { return FunctionAnalysisManagerCGSCCProxy(FAM); });<br>
+  CGAM.registerPass([&] { return ModuleAnalysisManagerCGSCCProxy(MAM); });<br>
+  FAM.registerPass([&] { return CGSCCAnalysisManagerFunctionProxy(CGAM); });<br>
+  FAM.registerPass([&] { return ModuleAnalysisManagerFunctionProxy(MAM); });<br>
+<br>
+  ModulePassManager MPM(/*DebugLogging*/ true);<br>
+  int ModulePassRunCount1 = 0;<br>
+  MPM.addPass(TestModulePass(ModulePassRunCount1));<br>
+<br>
+  CGSCCPassManager CGPM1(/*DebugLogging*/ true);<br>
+  int SCCPassRunCount1 = 0;<br>
+  int AnalyzedInstrCount1 = 0;<br>
+  int AnalyzedSCCFunctionCount1 = 0;<br>
+  int AnalyzedModuleFunctionCount1 = 0;<br>
+  CGPM1.addPass(TestSCCPass(SCCPassRunCount1, AnalyzedInstrCount1,<br>
+                            AnalyzedSCCFunctionCount1,<br>
+                            AnalyzedModuleFunctionCount1));<br>
+<br>
+  FunctionPassManager FPM1(/*DebugLogging*/ true);<br>
+  int FunctionPassRunCount1 = 0;<br>
+  FPM1.addPass(TestFunctionPass(FunctionPassRunCount1));<br>
+  CGPM1.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM1)));<br>
+  MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM1)));<br>
+<br>
+  MPM.run(*M, &MAM);<br>
+<br>
+  EXPECT_EQ(1, ModulePassRunCount1);<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Perhaps it's obvious to readers in this domain, but it's not clear to me why these numbers are the right ones - perhaps a comment explaining the test scenario would be helpful (maybe at the top by the module IR, or down here by the EXPECTs, not sure)? Or isolating the tests a bit more (refactoring so as not to have to repeat too much boilerplate setup) so each one is more obvious?</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+  EXPECT_EQ(1, ModuleAnalysisRuns);<br>
+  EXPECT_EQ(4, SCCAnalysisRuns);<br>
+  EXPECT_EQ(6, FunctionAnalysisRuns);<br>
+<br>
+  EXPECT_EQ(4, SCCPassRunCount1);<br>
+  EXPECT_EQ(14, AnalyzedInstrCount1);<br>
+  EXPECT_EQ(6, AnalyzedSCCFunctionCount1);<br>
+  EXPECT_EQ(4 * 6, AnalyzedModuleFunctionCount1);<br>
+}<br>
+<br>
+}<br>
<br>
Modified: llvm/trunk/unittests/Analysis/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/CMakeLists.txt?rev=261624&r1=261623&r2=261624&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/CMakeLists.txt?rev=261624&r1=261623&r2=261624&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Analysis/CMakeLists.txt (original)<br>
+++ llvm/trunk/unittests/Analysis/CMakeLists.txt Tue Feb 23 04:02:02 2016<br>
@@ -9,6 +9,7 @@ add_llvm_unittest(AnalysisTests<br>
   AliasAnalysisTest.cpp<br>
   CallGraphTest.cpp<br>
   CFGTest.cpp<br>
+  CGSCCPassManagerTest.cpp<br>
   LazyCallGraphTest.cpp<br>
   ScalarEvolutionTest.cpp<br>
   MixedTBAATest.cpp<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div></blockquote></div>