[PassBuilder] promote pass-pipeline parsing API to public

[fedor.sergeev]

Summary

While currently available pipeline parsing API (PassBuilder::parsePassPipeline)
is enough for parsing textual representation of a default pipeline,
it becomes insufficient when parsing pipelines for complex custom passes/pass
managers that take nested pipelines. This parsing is being performed by callbacks
(e.g. moudle pipeline parsing callback) which are passed the pass name + array of
pre-parsed pipeline elements. Now after matching the name in callback user has to
process pipeline elements and there is no public API for that.

Whe have methods that take ArrayRef<PipelineElement> - parse*PassPipeline,
but these methods are private.

Just promoting these APIs to public.

[philip.pfaffe]

What is your specific usecase for this? You have a custom PassManager which can contain, e.g., a function pass pipeline?

[chandlerc]

FWIW, I'm not really a fan of this.

I feel like this is really an implementation detail of the pass builder.

I echo Philip's question: what's the specific use case? Without knowing that, it is hard to suggest a good alternative path forward.

[fedor.sergeev]

In D60814#1469844, @philip.pfaffe wrote:

What is your specific usecase for this? You have a custom PassManager which can contain, e.g., a function pass pipeline?

Right. We are building our own "kinda-SCC" pass manager, which runs inlining-devirtualization iteration doing walk over functions in rather different order than current SCC does.
This pass manager right now accepts at least one function pass pipeline (and we think about adding one more), and I would like to be able to populate this pass manager from -passes.

So, I'm using a module-pipeline parsing callback:

bool parseOurPipeline(StringRef Name, ModulePassManager &PM,
                      ArrayRef<PassBuilder::PipelineElement> InnerPipeline,
                      PassBuilder &PB) {
   if (Name == "our-pass-manager") {
      FunctionPassManager SimplifyPM;
      if (auto Err = PB.parseFunctionPassPipeline(SimplifyPM, InnerPipeline, false, false))
       return false;
     PM.addPass(OurPassManager(std::move(SimplifyPM)));
     return true;
  }
}

and then

PB.registerPipelineParsingCallback(
      [&PB](StringRef Name, ModulePassManager &PM,
          ArrayRef<PassBuilder::PipelineElement> InnerPipeline) {
      return parseOurPipeline(Name, PM, InnerPipeline, PB);
     });

So thats basically a problem of introducing a custom pass manager that can be fully handled by -passes=.

I agree that it looks like exposing implementation detail.
But then, isnt passing ArrayRef<PipelineElement> to pipeline-parsing callback already exposing this implementation detail?

[chandlerc]

In D60814#1469963, @fedor.sergeev wrote:

In D60814#1469844, @philip.pfaffe wrote:

What is your specific usecase for this? You have a custom PassManager which can contain, e.g., a function pass pipeline?

Right. We are building our own "kinda-SCC" pass manager, which runs inlining-devirtualization iteration doing walk over functions in rather different order than current SCC does.
This pass manager right now accepts at least one function pass pipeline (and we think about adding one more), and I would like to be able to populate this pass manager from -passes.

So, I'm using a module-pipeline parsing callback:

bool parseOurPipeline(StringRef Name, ModulePassManager &PM,
                      ArrayRef<PassBuilder::PipelineElement> InnerPipeline,
                      PassBuilder &PB) {
   if (Name == "our-pass-manager") {
      FunctionPassManager SimplifyPM;
      if (auto Err = PB.parseFunctionPassPipeline(SimplifyPM, InnerPipeline, false, false))
       return false;
     PM.addPass(OurPassManager(std::move(SimplifyPM)));
     return true;
  }
}

and then

PB.registerPipelineParsingCallback(
      [&PB](StringRef Name, ModulePassManager &PM,
          ArrayRef<PassBuilder::PipelineElement> InnerPipeline) {
      return parseOurPipeline(Name, PM, InnerPipeline, PB);
     });

So thats basically a problem of introducing a custom pass manager that can be fully handled by -passes=.

I agree that it looks like exposing implementation detail.
But then, isnt passing ArrayRef<PipelineElement> to pipeline-parsing callback already exposing this implementation detail?

I see.

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

[fedor.sergeev]

In D60814#1470299, @chandlerc wrote:

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

It might be a bit conceptually cleaner, but other than renaming the object from PassBuilder to PassPipelineParser (and internally talking to PassBuilder), what would be the API difference
compared to what is being suggested here?
I see a bunch of questions, e.g. - should we keep the callbacks in the parser object? etc
And I dont really see what practical benefits would we get from separating the parsing API out...

Said that, I'm fine with going this direction if it appears to be useful.

[philip.pfaffe]

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

By main parsing API you mean the part the turns the pipeline tree into the PM tree, right? I think that makes a lot of sense. That way, we get to keep the PassBuilder API surface small and the tree-parsing API doesn't leak into the application that just wants PassManagers. I don't think it's really important whether PassBuilder keeps ownership of the callbacks or becomes a thin registration proxy, but the latter is probably simpler?

I'd very much prefer that path to abstracting away the array-ref API. The particular callbacks that get exposed to this API are super powerful since they get to take over parsing the pipeline tree. Personally I'm fine if the API is a bit less abstract there.

[chandlerc]

In D60814#1470310, @fedor.sergeev wrote:

In D60814#1470299, @chandlerc wrote:

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

It might be a bit conceptually cleaner, but other than renaming the object from PassBuilder to PassPipelineParser (and internally talking to PassBuilder), what would be the API difference
compared to what is being suggested here?

My primary goal is to separate the API that does detailed parsing from the high-level API, and make the detailed one only used by callbacks. This keeps the widely *used* API as *narrow* as possible, and only exposes the wide API to the specialized users.

I see a bunch of questions, e.g. - should we keep the callbacks in the parser object? etc

See my reply to Philip.

And I dont really see what practical benefits would we get from separating the parsing API out...

Again, its about keeping different layers of APIs clean and minimal.

Imagine we want to change the parsing code at some point. This will ensure only users registering callbacks that *do custom parsing* will be impacted, rather than having unexpected users be impacted in ways that are surprising / confusing.

In D60814#1470327, @philip.pfaffe wrote:

By main parsing API you mean the part the turns the pipeline tree into the PM tree, right? I think that makes a lot of sense. That way, we get to keep the PassBuilder API surface small and the tree-parsing API doesn't leak into the application that just wants PassManagers. I don't think it's really important whether PassBuilder keeps ownership of the callbacks or becomes a thin registration proxy, but the latter is probably simpler?

I think I would much rather do this differently.

Move these methods to a nested class whose only job is parsing. Make the constructors and destructor private and befriend the pass builder. Sink the pass builder state that is used in parsing into this object rather than the pass builder itself. Pass a reference to this object to the callback for use in parsing.

I'd keep everything except for the actual parsing logic in the pass builder where it already is.

This basically isolates the chunk of the pass builder's state and API necessary to do custom parsing and makes it available in the only context where we support it. It seems nicely narrow, but also will lend itself to any future growth needed to support more interesting custom parsing.

I'd very much prefer that path to abstracting away the array-ref API. The particular callbacks that get exposed to this API are super powerful since they get to take over parsing the pipeline tree. Personally I'm fine if the API is a bit less abstract there.

Yeah, as long as it is layered so that the two things aren't commingled and easily misused, I agree.

[fedor.sergeev]

see D60870 for a proof-of-concept implementation of PipelineParser.

llvm/include/llvm/Passes/PassBuilder.h

public:
   /// 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>,
                                bool VerifyEachPass, bool DebugLogging)> &C) {
     TopLevelPipelineParsingCallbacks.push_back(C);
   }
 
+  Error parseLoopPassPipeline(LoopPassManager &LPM,
+                              ArrayRef<PipelineElement> Pipeline,
+                              bool VerifyEachPass, bool DebugLogging);
+  Error parseFunctionPassPipeline(FunctionPassManager &FPM,
+                                  ArrayRef<PipelineElement> Pipeline,
+                                  bool VerifyEachPass, bool DebugLogging);
+  Error parseCGSCCPassPipeline(CGSCCPassManager &CGPM,
+                               ArrayRef<PipelineElement> Pipeline,
+                               bool VerifyEachPass, bool DebugLogging);
+  Error parseModulePassPipeline(ModulePassManager &MPM,
+                                ArrayRef<PipelineElement> Pipeline,
+                                bool VerifyEachPass, bool DebugLogging);
 private:
   static Optional<std::vector<PipelineElement>>
   parsePipelineText(StringRef Text);
 
   Error parseModulePass(ModulePassManager &MPM, const PipelineElement &E,
                         bool VerifyEachPass, bool DebugLogging);
   Error parseCGSCCPass(CGSCCPassManager &CGPM, const PipelineElement &E,
                        bool VerifyEachPass, bool DebugLogging);
   Error parseFunctionPass(FunctionPassManager &FPM, const PipelineElement &E,
                           bool VerifyEachPass, bool DebugLogging);
   Error parseLoopPass(LoopPassManager &LPM, const PipelineElement &E,
                       bool VerifyEachPass, bool DebugLogging);
   bool parseAAPassName(AAManager &AA, StringRef Name);
 
-  Error parseLoopPassPipeline(LoopPassManager &LPM,
-                              ArrayRef<PipelineElement> Pipeline,
-                              bool VerifyEachPass, bool DebugLogging);
-  Error parseFunctionPassPipeline(FunctionPassManager &FPM,
-                                  ArrayRef<PipelineElement> Pipeline,
-                                  bool VerifyEachPass, bool DebugLogging);
-  Error parseCGSCCPassPipeline(CGSCCPassManager &CGPM,
-                               ArrayRef<PipelineElement> Pipeline,
-                               bool VerifyEachPass, bool DebugLogging);
-  Error parseModulePassPipeline(ModulePassManager &MPM,
-                                ArrayRef<PipelineElement> Pipeline,
-                                bool VerifyEachPass, bool DebugLogging);
-
   void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
                          OptimizationLevel Level, bool RunProfileGen, bool IsCS,
                          std::string ProfileFile,
                          std::string ProfileRemappingFile);
 
   void invokePeepholeEPCallbacks(FunctionPassManager &, OptimizationLevel);
 
   // Extension Point callbacks