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

First round of high-level feedback on this approach, thanks so much for working no this!



================
Comment at: include/llvm/Passes/PassBuilder.h:323
+
+public:
+  enum class ExtensionPoint {
----------------
Generally, I'd like to not add more transitions between public and private. I would prefer the following rough order:

0. Before the first public, any private types / aliases needed to define the public API.
1. Public API
2. Protected API (if it exists)
3. Private API


================
Comment at: include/llvm/Passes/PassBuilder.h:324
+public:
+  enum class ExtensionPoint {
+    /// EarlyAsPossible - This extension point allows adding passes before
----------------
Could you sort these based on the order in which they first are used in the default pipeline?


================
Comment at: include/llvm/Passes/PassBuilder.h:352-355
+    /// EnabledOnOptLevel0 - This extension point allows adding passes that
+    /// should not be disabled by O0 optimization level. The passes will be
+    /// inserted after the inlining pass.
+    EnabledOnOptLevel0,
----------------
This seems redundant with `OptimizerLast`. I think in the new PM we should just have `OptimizerLast` and we should pass the `OptimizationLevel` as a parameter to *all* of these callbacks.


================
Comment at: include/llvm/Passes/PassBuilder.h:377-386
+  template <typename PassManagerT>
+  using EPCallback = std::function<void(ExtensionPoint, PassManagerT &)>;
+  template <typename PassManagerT>
+  using PassCallback = std::function<bool(StringRef Name, PassManagerT &)>;
+  template <typename PassManagerT>
+  using PipelineCallback =
+      std::function<bool(StringRef Name, PassManagerT &,
----------------
What do you think about naming these `FooT`-style? I don't feel strongly about it, but often like it to identify pure type aliases.


================
Comment at: include/llvm/Passes/PassBuilder.h:380-384
+  using PassCallback = std::function<bool(StringRef Name, PassManagerT &)>;
+  template <typename PassManagerT>
+  using PipelineCallback =
+      std::function<bool(StringRef Name, PassManagerT &,
+                         ArrayRef<PassBuilder::PipelineElement> Pipeline)>;
----------------
I don't think it makes sense to have these be two different kinds of callbacks. At least, not until there are complaints about how annoying it is to ignore the rest.

I think your second signature is pretty good. It lets users that don't care about nested pipelines just use an unnamed parameter and ignore even the struct type, etc.

I would call this callback type something to do with *parsing* though as that is all it does.


================
Comment at: include/llvm/Passes/PassBuilder.h:386
+  template <typename AnalysisManagerT>
+  using AnalysisCallback = std::function<void(AnalysisManagerT &)>;
+
----------------
This should be called something to do with *registering*.


================
Comment at: include/llvm/Passes/PassBuilder.h:407-414
+  /// \brief Call all loaded plugins
+  ///
+  /// For all plugins regeistered with the PluginLoader, this method will call
+  /// the plugins' registration entrypoint, if it exists. The entrypoint is a
+  /// method with the extern "C" signature `void
+  /// RegisterPluginPasses(PassBuilder &PB)`.
+  /// Plugin passes can register callbacks with this PassBuilder instance.
----------------
I would suggest separating this into a later change. The extension mechanisms should work well out of the box for non-plugin use cases and it lets the review be a bit more focused.

As an example, I'd like to use some of this to implement support for using the new PM in Clang when running sanitizers.


================
Comment at: include/llvm/Passes/PassBuilder.h:422-425
+  void registerEPCallback(const CGSCCEPCallback &C);
+  void registerEPCallback(const FunctionEPCallback &C);
+  void registerEPCallback(const LoopEPCallback &C);
+  void registerEPCallback(const ModuleEPCallback &C);
----------------
Rather than all four of these plus the huge swath of extension point enums above, how do you feel about a different API:

All of the extension points above have a fixed pass manager type. So what if we have one `registerFooCallback` for each extension point, completely remove the enum from the public API, and make the pass manager type concrete and precise?

While this would mean more `register...` functions, not more than we already have enumerators. You should move the comments currently on the enumerators to be on the registration routines. I also generally think it would be best to document the extension points separately from the rest of the registration stuff as its much less boiler-plate-y.

One reason why I prefer this -- with the current API, there are a lot of combinations of pass manager types and enumerations that won't get any compile time error but cannot actually fire in practice.


================
Comment at: include/llvm/Passes/PassBuilder.h:436-462
+  /// {{@ Run all registered callbacks for a given pass type
+  void invokeAnalysisCallbacks(CGSCCAnalysisManager &PM) const;
+  void invokeAnalysisCallbacks(FunctionAnalysisManager &PM) const;
+  void invokeAnalysisCallbacks(LoopAnalysisManager &PM) const;
+  void invokeAnalysisCallbacks(ModuleAnalysisManager &PM) const;
+  void invokeEPCallbacks(ExtensionPoint EP, CGSCCPassManager &PM) const;
+  void invokeEPCallbacks(ExtensionPoint EP, FunctionPassManager &PM) const;
----------------
Why are these public? I would make them implementation details. And in most cases they're called in only one place and could be completely inlined.


================
Comment at: include/llvm/Passes/PassBuilder.h:489-491
+/// This utility template takes care of adding require<> and invalidate<>
+/// passes for an analysis to a given \c PassManager. It is intended to be used
+/// during parsing of a pass pipeline when parsing a single PipelineEntry.
----------------
I think you should show an example of how to use this here.


================
Comment at: include/llvm/Passes/PassBuilder.h:494
+          typename... ExtraArgTs>
+bool HandleAnalysisUtilities(
+    StringRef AnalysisName, StringRef PipelineEntry,
----------------
Naming convention suggests fooBarBaz here.

Also, "handle" doesn't really help the reader much IMO. I would name it after the name of the callback above. IE, if above it is something like `ParsePassCallbackT` I would call this `parseAnalysisUtilityPasses`.


================
Comment at: include/llvm/Passes/PassBuilder.h:495
+bool HandleAnalysisUtilities(
+    StringRef AnalysisName, StringRef PipelineEntry,
+    PassManager<IRUnitT, AnalysisManagerT, ExtraArgTs...> &PM) {
----------------
There is a type `PipelineEntry`. I don't think that makes a good parameter name here, especially when the parameter is extracted *from* that type...


https://reviews.llvm.org/D33464