[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.

Your testing strategy looks good. Comments below. I think this is getting close enough that it makes sense to start hacking on tests for it. Thanks so much for all the work here, I really like how this is turning out!



================
Comment at: include/llvm/Passes/PassBuilder.h:369
+  ///
+  /// ModuleOptimizerEarly - This extension point allows adding passes
+  /// just before the main module-level optimization passes.
----------------
Here and below, can drop the old enumerator before the '-'.


================
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
----------------
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.


================
Comment at: include/llvm/Passes/PassBuilder.h:466
+
+  /// \brief Register a callback for an AA Name
+  void registerParseAACallback(
----------------
I would expand on this a bit.... You should spell out what 'AA' stands for here, and talk about what the callback is expected to do with the `AAManager`.


================
Comment at: include/llvm/Passes/PassBuilder.h:485
+  /// instance
+  void registerRegisterAnalysisCallback(
+      const std::function<void(CGSCCAnalysisManager &)> &C) {
----------------
I think `registerAnalysisRegistrationCallback` reads slightly better... But not a lot...


================
Comment at: include/llvm/Passes/PassBuilder.h:504
+  /// {{@ Register pipeline parsing callbacks with this pass builder instance.
+  void registerParsePipelineCallback(
+      const std::function<bool(StringRef, CGSCCPassManager &,
----------------
Similarly `registerPipelineParsingCallback`. Curious if others agree though.


================
Comment at: lib/Passes/PassBuilder.cpp:1059
 
-static bool isModulePassName(StringRef Name) {
+template <typename PassManagerT, typename CallbacksArrayT>
+static bool callbacksAcceptPassName(StringRef Name,
----------------
chandlerc wrote:
> Probably worth a comment explaining what craziness we're doing here and why. =]
Here and below, I would just s/`CallbacksArrayT`/`CallbacksT`/g. I don't think `Array` is really helping... You really mean more `Range`, but I think plural is enough to signify "hey I can loop over it".


================
Comment at: lib/Passes/PassBuilder.cpp:1059-1060
 
-static bool isModulePassName(StringRef Name) {
+template <typename PassManagerT, typename CallbacksArrayT>
+static bool callbacksAcceptPassName(StringRef Name,
+                                    CallbacksArrayT &Callbacks) {
----------------
Probably worth a comment explaining what craziness we're doing here and why. =]


================
Comment at: lib/Passes/PassBuilder.cpp:1267-1275
+  }
+
+  for (auto &C : ParseModulePipelineCallbacks)
+    if (C(Name, MPM, InnerPipeline))
+      return true;
+
+  if (!InnerPipeline.empty())
----------------
This restructuring seems bad to me...

I think we always want to run the callbacks *last*. The way the parsing logic works, there are two "last" places (one when we have an inner pipeline, one without).

If you're bothered by duplicating the for loop, you could put this in a tiny lambda you declare at the top, and just call it on each path?


================
Comment at: lib/Passes/PassBuilder.cpp:1647
       Pipeline = {{"function", {{"loop", std::move(*Pipeline)}}}};
-    else
+    else {
+
----------------
If we're going to brace one arm, let's brace them all here.


https://reviews.llvm.org/D33464