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

Generally this is starting to get pretty close. What are your thoughts on testing? I would suggest a unittest because that should be a good way to simulate an "out of tree" user.



================
Comment at: include/llvm/Passes/PassBuilder.h:49
 public:
+  /// A struct to capture parsed pass pipeline names.
+  struct PipelineElement {
----------------
Now that this is public, probably need to flesh out its documentation so folks understand the details of how this should be expected to work.


================
Comment at: include/llvm/Passes/PassBuilder.h:141-142
 
+  template <typename PassManagerT>
+  using EPCallbackT = std::function<void(PassManagerT &, OptimizationLevel)>;
+  template <typename PassManagerT>
----------------
I think this template and all the using aliases of it below don't really help readability enough. These types only show up in one place (the declaration of a function that has a nice comment) and should just be written out in that location IMO.


================
Comment at: include/llvm/Passes/PassBuilder.h:171
+      std::function<bool(ModulePassManager &, ArrayRef<PipelineElement>)>;
+
   explicit PassBuilder(TargetMachine *TM = nullptr,
----------------
mehdi_amini wrote:
> That's a long block of undocumented code here.
> 
Actually, I think all of these are used in essentially two places: a register signature and the vector declaration below. I'm not sure adding an alias just to save that single repeatition is worth it.

I actually find the public functions more readable without the type alias, even though it means a mild amount of repeatition in the container type at the bottom. What do you think Mehdi?


================
Comment at: include/llvm/Passes/PassBuilder.h:401
+  /// {{@ Register generic callbacks with this pass builder instance.
+  void registerParseAACallback(const ParseAACallbackT &C);
+  void
----------------
This one deserves its own dedicated comment rather than being grouped.


================
Comment at: include/llvm/Passes/PassBuilder.h:402-408
+  void
+  registerRegisterAnalysisCallback(const RegisterCGSCCAnalysisCallbackT &C);
+  void
+  registerRegisterAnalysisCallback(const RegisterFunctionAnalysisCallbackT &C);
+  void registerRegisterAnalysisCallback(const RegisterLoopAnalysisCallbackT &C);
+  void
+  registerRegisterAnalysisCallback(const RegisterModuleAnalysisCallbackT &C);
----------------
I would separate the analysis callbacks from the pipeline parsing callbacks and give them (each) their own documentation.


================
Comment at: include/llvm/Passes/PassBuilder.h:413-414
+  void registerParsePipelineCallback(const ParseModulePipelineCallbackT &C);
+  void registerParseTopLevelPipelineCallback(
+      const ParseTopLevelPipelineCallbackT &C);
+  /// @}}
----------------
This one too deserves its own documentation IMO.


================
Comment at: lib/Passes/PassBuilder.cpp:275-278
+void PassBuilder::registerEarlyAsPossibleEPCallback(
+    const FunctionEPCallbackT &C) {
+  EarlyAsPossibleEPCallbacks.push_back(C);
+}
----------------
Since these are all one-line functions, would it make sense to inline them into the header so we don't have to re-declare all of them?


================
Comment at: lib/Passes/PassBuilder.cpp:875
   // calls, etc, so let instcombine do this.
-  // FIXME: add peephole extensions here as the legacy PM does.
-  MPM.addPass(createModuleToFunctionPassAdaptor(InstCombinePass()));
+  FunctionPassManager SimplifyFPM(DebugLogging);
+  SimplifyFPM.addPass(InstCombinePass());
----------------
`SimplifyFPM` -> `PeepholeFPM` ?


================
Comment at: lib/Passes/PassBuilder.cpp:899-902
   FPM.addPass(InstCombinePass());
+
+  for (auto &C : PeepholeEPCallbacks)
+    C(FPM, Level);
----------------
We should factor out a helper that adds the peephole passes to an FPM as we do this in a lot of places.


https://reviews.llvm.org/D33464