[PATCH] D60814: [PassBuilder] promote pass-pipeline parsing API to public

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In D60814#1470310 <https://reviews.llvm.org/D60814#1470310>, @fedor.sergeev wrote:

> In D60814#1470299 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60814/new/

https://reviews.llvm.org/D60814