[PATCH] D33464: [PM][WIP] Enable out-of-tree registration of passes with the new PassBuilder

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Whew, back to this.

Mostly nit picks and comments about testing.



================
Comment at: include/llvm/Passes/PassBuilder.h:480-489
+  /// \brief Register a callback for a top-level pipeline entry.
+  ///
+  /// If the PassManager type is not given at the top level of the pipeline
+  /// text, this Callback should be used to determine the appropriate stack of
+  /// PassManagers and populate the passed ModulePassManager.
+  void registerParseTopLevelPipelineCallback(
+      const std::function<bool(ModulePassManager &, ArrayRef<PipelineElement>,
----------------
I would sink this to after the other pipeline parsing callback registration entry points.


================
Comment at: include/llvm/Passes/PassBuilder.h:572
+
+  // EP callbacks
+  SmallVector<std::function<void(ModulePassManager &, OptimizationLevel)>, 2>
----------------
EP -> Extension Point.


================
Comment at: include/llvm/Passes/PassBuilder.h:371-385
+  void registerModuleOptimizerEarlyEPCallback(
+      const std::function<void(ModulePassManager &, OptimizationLevel)> &C) {
+    ModuleOptimizerEarlyEPCallbacks.push_back(C);
+  }
+
+  /// \brief Register a callback for a default optimizer pipeline extension
+  /// point
----------------
philip.pfaffe wrote:
> chandlerc wrote:
> > I think we should drop both of these "early" extension points and the "last" extension point.
> > 
> > In the new PM, it is trivial to nest a pass manager, allowing users to easily run passes both before and after the pipeline. This should reduce the number of extension points we have to carry and it removes a semantic complication with the "last" extension point. If we really can't get any of the users of these ported over, we can add them back, but I'd like to be somewhat lazy here and work to address these use cases with nesting and wrapping as long as possible.
> I'm not entirely convinced about this. I expect most users of the callbacks won't have access to the outermost PassManagers, so nesting will not be easily possible.
How so? I'm curious what the issue is here... Callbacks seem like the most complex form of extension, whereas nesting seems much simpler...


================
Comment at: lib/Passes/PassBuilder.cpp:990
+
+  invokePeepholeEPCallbacks(MainFPM, Level);
   MainFPM.addPass(JumpThreadingPass());
----------------
Here especially (but also elsewhere) I'd put the `invokePeepholeEP...` line adjacent to the `InstCombinePass` line (they're the two peepholes) rather than next to the subsequent stuff which is unrelated.


================
Comment at: lib/Passes/PassBuilder.cpp:1312
+    if (L == O0) {
+
+      for (auto &C : OptimizerLastEPCallbacks)
----------------
No need for blank line here.


================
Comment at: lib/Passes/PassBuilder.cpp:1571-1574
+  for (auto &C : AAParsingCallbacks)
+    if (C(Name, AA))
+      return true;
+
----------------
Should this go after we parse the builtin in AA names?


================
Comment at: lib/Passes/PassBuilder.cpp:1678
+    } else {
+
+      for (auto &C : TopLevelPipelineParsingCallbacks)
----------------
No need for blank line.


================
Comment at: lib/Passes/PassBuilder.cpp:1699-1708
+  StringRef FirstName = Pipeline->front().Name;
+  if (!isCGSCCPassName(FirstName, CGSCCPipelineParsingCallbacks)) {
+    if (isFunctionPassName(FirstName, FunctionPipelineParsingCallbacks)) {
+      Pipeline = {{"function", std::move(*Pipeline)}};
+    } else if (isLoopPassName(FirstName, LoopPipelineParsingCallbacks)) {
+      Pipeline = {{"function", {{"loop", std::move(*Pipeline)}}}};
+    } else {
----------------
I'm not sure you want this fallback logic here...

If you want to be able to essentially duplicate the above logic, you want this to *only* succeed at parsing a CGSCC pass pipeline.

Otherwise, you'll never be able to skip the CGSCC layer when you get a function pass (or something nested inside a function pass) and you're trying to build the module layer thing.


================
Comment at: lib/Passes/PassBuilder.cpp:1722-1729
+  StringRef FirstName = Pipeline->front().Name;
+  if (!isFunctionPassName(FirstName, FunctionPipelineParsingCallbacks)) {
+    if (isLoopPassName(FirstName, LoopPipelineParsingCallbacks)) {
+      Pipeline = {{"loop", std::move(*Pipeline)}};
+    } else {
+      return false;
+    }
----------------
While less important, same comment as above.


================
Comment at: test/Other/new-pm-defaults.ll:29-33
+; RUN: opt -disable-verify -debug-pass-manager \
+; RUN:     -passes-ep-module-optimizer-early='no-op-module' \
+; RUN:     -passes='default<O3>' -S  %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-O --check-prefix=CHECK-O3 \
+; RUN:     --check-prefix=CHECK-EP-ModuleEarly
----------------
What about having just one `opt` invocation with a no-op pass in *every* extension point? I don't think you lose any coverage, and then you have a much easier check prefix of just `CHECK-EP`.


================
Comment at: tools/opt/NewPMDriver.cpp:101-105
+#ifdef LINK_POLLY_INTO_TOOLS
+namespace polly {
+void RegisterPollyPasses(PassBuilder &);
+}
+#endif
----------------
Do the polly bits in a follow-up commit?


================
Comment at: unittests/IR/HookManagerTest.cpp:23
+namespace {
+/// A test analysis, counting its runs and invalidations
+template <typename IRUnitT, typename AnalysisManagerT>
----------------
Rather than counting things, I would suggest using gmock for this. It is kind of what gmock was built for. To see how to get value-type wrappers around mocks that you can set expectations on, look at `unittests/Transforms/Scalar/LoopPassManagerTest.cpp`. You should end up with very similar code to this to template out the mocks over the types you care about, but should be able to use things like EXPECT_CALL to track when 'invalidate' or 'run' are called which seems way better than counting.


================
Comment at: unittests/IR/HookManagerTest.cpp:83-113
+/// Wrapper class for the registration and parsing callbacks
+template <typename IRUnitT, typename AnalysisManagerT, typename PassManagerT>
+struct Callback {
+  /// Handle analysis registration
+  void operator()(AnalysisManagerT &AM) {
+    int *AnaRun = AnalysesRun;
+    int *AnaInv = AnalysesInvalidated;
----------------
I think you can inline all this as lambas below? You shouldn't need a generic lambda because you should have the types inside that template already.


================
Comment at: unittests/IR/HookManagerTest.cpp:122-127
+/// Test fixture
+/// Template specialization to extract the IRUnit and AM types from a
+/// PassManagerT
+template <typename _IRUnitT, typename _AnalysisManagerT, typename... ExtraArgTs>
+class HookManagerTest<PassManager<_IRUnitT, _AnalysisManagerT, ExtraArgTs...>>
+    : public testing::Test {
----------------
Throughout this test, I would try to comment a bit more heavily. My suggestion would be the following: assume the reader is trying to learn how to use the extension point interface to write their own customizations, and they are relatively unfamiliar with the extension point API, googletest (especially typed tests) and goolemock. You want to have enough comments that they can read this code cold and use it as documentation for how the API should work in practice (as opposed to its spec).


Also, probably need a better name than `HookManagerTest`?


https://reviews.llvm.org/D33464