We should have a deprecation timeline that revolves around the major releases. I’d say we announce the legacy PM for the optimization pipeline to be deprecated the major release after the new PM has been the default. Looks like the new PM switch made it into LLVM 12.
The major blocker right now is that the C API doesn’t have hooks into the new PM infrastructure. llvm/include/llvm-c/Transforms/PassManagerBuilder.h should be fairly straightforward to port to the new PM, but the API to add individual passes (e.g. llvm/include/llvm-c/Transforms/Scalar.h) needs to distinguish between the different types of passes, e.g. module vs function pass. The new PM has explicit pass nesting, so we’ll need to make sure that we add a function pass to a function pass manager. Or we could simplify things and force each pass added via the C API to run in isolation (e.g. two adjacent function passes would run completely separately rather than being interleaved function-by-function), which doesn’t match how pipelines are constructed everywhere else, but it’s already an adhoc API.
At some point after the deprecation announcement we should start cleaning up tests for passes in the optimization pipeline to use opt -passes=foo rather than opt -foo.
I don’t think the new PM switch is part of LLVM 12. The change to make it the default landed on February 3rd, which was after the branch cut. If you examine llvm/CMakeLists.txt on the release/12.x branch, you’ll also see that ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER still defaults to FALSE.
For a downstream maintainer that is doing regular merges/rebases toward main (kind of on a daily basis) it is really nice to have a timeline for this.
A bit unclear to me what your proposal would mean though. Is it that after the LLVM 13 branch out we could expect that the support for Legacy PM will start to “disappear” in opt on the main branch (i.e. when the version on main branch is stepped to 14, which will happen before the LLVM 13 release)?
When it comes to “disappear” I figure that:
At some point lit tests, today running new-pm by default, will be converted to use -passes.
(This should not be a big problem for myself at least. We’ve actually started to build with the default setting for ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER, to make sure we use the default for “upstream” tests/targets, but we are forcing the legacy PM by an override in opt.cpp for our downstream target.)
At some point lit tests will stop testing legacy PM completely (no more using -enable-new-pm=0).
(This is a point in the timeline that could be interesting for us, as we probably want to have switched over into using new PM downstream before quality of legacy PM this.)
At some point passes that are specific to Legacy PM will be removed.
(If I need then downstream I’d probably need to maintain them myself just like any downstream pass, so shouldn’t be a big problem. However, we probably want to aim for switching to use new PM downstream before this happens.)
At some point in time flags/options like -enable-new-pm and ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER will be dropped, along with support in opt.cpp to use the legacy PM.
(Here I think that we definitely want to have switched to use new PM downstream, because maintaining that OOT is probably more costly.)
Is the plan that all those things will start to happen more or less at once?
I wouldn’t mind looking into hooking the new PM infrastructure into the LLVM C API. However, I would like to know a little more about the task. I’ve had a look at the new PM infrastructure, and from taking a quick glance at the available APIs and how it’s used in the tests I assume we’d look to do something similar to the existing legacy PM hooks.
Things are a bit different as far as I’ve understood because we have Module, CGSCC, Function, and Loop passes, each of which may be adapted into “higher” kinds, eg FunctionPM → ModulePM, all of which differs a bit from the legacy PM. From what I’ve gathered, we’ll need to set up the PMs themselves as you mentioned and we’ll have to hook a set (all?) of the new PM passes into this. Would it be possible to get a more comprehensive list of what needs to be done?
I’m pretty new around here so I’ll probably require some guidance further down the line.
For a downstream maintainer that is doing regular merges/rebases toward main (kind of on a daily basis) it is really nice to have a timeline for this.
A bit unclear to me what your proposal would mean though. Is it that after the LLVM 13 branch out we could expect that the support for Legacy PM will start to “disappear” in opt on the main branch (i.e. when the version on main branch is stepped to 14, which will happen before the LLVM 13 release)?
When it comes to “disappear” I figure that:
At some point lit tests, today running new-pm by default, will be converted to use -passes.
(This should not be a big problem for myself at least. We’ve actually started to build with the default setting for ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER, to make sure we use the default for “upstream” tests/targets, but we are forcing the legacy PM by an override in opt.cpp for our downstream target.)
At some point lit tests will stop testing legacy PM completely (no more using -enable-new-pm=0).
(This is a point in the timeline that could be interesting for us, as we probably want to have switched over into using new PM downstream before quality of legacy PM this.)
At some point passes that are specific to Legacy PM will be removed.
(If I need then downstream I’d probably need to maintain them myself just like any downstream pass, so shouldn’t be a big problem. However, we probably want to aim for switching to use new PM downstream before this happens.)
At some point in time flags/options like -enable-new-pm and ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER will be dropped, along with support in opt.cpp to use the legacy PM.
(Here I think that we definitely want to have switched to use new PM downstream, because maintaining that OOT is probably more costly.)
Is the plan that all those things will start to happen more or less at once?
That mostly sounds right to me.
We’d still have to have support in opt for the legacy PM to run IR passes that are part of the codegen pipeline. But we could still remove the -enable-new-pm flag after moving tests for passes in the optimization pipeline to use -passes.
Awesome, I’m happy to do reviews and answer any further questions.
I’m imagining a new PassBuilder.h in llvm/include/llvm-c where PassManagerBuilder.h was.
Initially I was thinking we’d need wrappers around the various types of pass managers as you’ve mentioned, as well as the adaptors (e.g. createModuleToFunctionPassAdaptor()). And each of the LLVMAdd*Pass() would need a corresponding new PM alternative that adds the pass to the proper pass manager type.
But now that I think about it some more, we might be able to bypass all of that and instead use the new PM’s pass pipeline text parsing (the same thing that opt -passes=foo uses). This seems exactly the sort of thing it would be useful for. We’ll bypass the need to create wrappers for the different types of pass managers since we just get back a ModulePassManager from the PassBuilder.
The code in NewPMDriver.cpp should be a good starting place to figure out what’s necessary to create a PassBuilder, then how to get a ModulePassManager from it and run it on some IR.
I imagine first you’ll need some way to setup the options to pass to the PassBuilder, so the C wrapper would probably own various things like the various *AnalysisManagers, PassInstrumentationCallbacks, StandardInstrumentations, and PipelineTuningOptions. Then the user can ask for some pipeline via a string, whether that’s an individual pass like “instcombine” or a full pipeline like “default”, and you create a PassBuilder from the options, then tell it to create the ModulePassManager via PassBuilder::parsePassPipeline().
It might be worth noting that the regression tests are currently
failing with ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER=OFF. If it is still
supported till the next release, they should be fixed.
Yes we should make sure tests pass with the legacy PM as the default, treating the bot like any other bot.
I’ve fixed the two func-attrs failures.The two OpenMP failures look like real issues under the legacy PM that need to be investigated.
Thank you Arthur, that sounds great. Using opt’s pass name parser sounds like a good plan and a great way to save ourselves from complications with the different PM types. We can then construct the ModulePassManager and run that over an LLVM module the user passes to us.
I’ll have a closer look at this over the next few days and I’ll possibly gather a tiny list of questions I may have before we get started.
Does the deprecation mean that individual IR passes might stop
supporting the legacy PM? If so I have a slight concern: at the moment
a target can pull arbitrary IR passes into the codegen pipeline. For
example, AMDGPU's addPreISel() hook does this:
addPass(createSinkingPass());
I guess most people would consider Sinking to be an optimization pass
that would only be used in the optimization pipeline, but that ain't
necessarily so. In fact I'm not sure if there is any reliable
definition of which IR passes are really codegen passes, is there?
Depreciation would inevitably mean that bugs affecting only the legacy
pass manager might not get fixed (unless a volunteer would do so).
Even today there are passes that are only added to the new pass
manager.
Greetings, I’ve had the time to take a better look at the code in the NewPMDriver.cpp source and I believe I have a good overview of the task now. Here are some things which I’m not too sure about so I’d like your input on this:
What are the PreservedAnalyses and how do we use them? Is it crucial for the LLVM-C API?
How do we want to handle the various callbacks for the PassInstrumentationCallbacks? They’re taking the templated IRUnit which could be different types upon each invocation.
Where do we want the binding code itself? Some places have files like ExecutionEngineBindings.cpp, while others have it at the end of files relevant to the code being hooked into.
I see the C++ API accepts PGO Options, should that be hooked into as well?
With that being said, I think I have a decent plan for the interface itself. We’ll provide a PassBuilder which we can construct with the PassInstrumentationCallbacks, PipelineTuningOptions, etc. The various *AnalysisManagers may also be added to the PassBuilder. The StandardInstrumentation can be added by passing a PassInstrumentationCallbacks and optionally a FunctionAnalysisManager like the C++ llvm::StandardInstrumentation::registerCallbacks method does.
The PassBuilder will then be able to accept a Module and an opt-style string with the passes to be used. A ModulePassManager will be constructed as you described and it will run the passes over the module.
Let me know what you think of an approach like this…
Greetings, I’ve had the time to take a better look at the code in the NewPMDriver.cpp source and I believe I have a good overview of the task now. Here are some things which I’m not too sure about so I’d like your input on this:
What are the PreservedAnalyses and how do we use them? Is it crucial for the LLVM-C API?
No, it’s more of an implementation detail that users don’t need to worry about.
How do we want to handle the various callbacks for the PassInstrumentationCallbacks? They’re taking the templated IRUnit which could be different types upon each invocation.
I’m not following. Isn’t it like all the other things you’re passing into the PassBuilder?
Where do we want the binding code itself? Some places have files like ExecutionEngineBindings.cpp, while others have it at the end of files relevant to the code being hooked into.
Separating it out into a different file from PassBuilder.cpp seems cleanest to me. Maybe right along side it in the same directory? We can always move it around later if we find somewhere better.
I see the C++ API accepts PGO Options, should that be hooked into as well?
Probably at some point, but that can come later. I don’t see where the legacy PM’s C API does that.
With that being said, I think I have a decent plan for the interface itself. We’ll provide a PassBuilder which we can construct with the PassInstrumentationCallbacks, PipelineTuningOptions, etc. The various *AnalysisManagers may also be added to the PassBuilder. The StandardInstrumentation can be added by passing a PassInstrumentationCallbacks and optionally a FunctionAnalysisManager like the C++ llvm::StandardInstrumentation::registerCallbacks method does.
The PassBuilder will then be able to accept a Module and an opt-style string with the passes to be used. A ModulePassManager will be constructed as you described and it will run the passes over the module.
The things that make the PassBuilder customizable are the PipelineTuningOptions, PGOOptions, and DebugLogging. We should probably do something like heap allocate a PTO/PGOOptions and provide an opaque pointer to it, and set those options, then pass it to the final build/run function. Sort of like LLVMPassManagerRef, but the legacy pass builder stored its options inside the pass builder itself, whereas the new pass builder has a separate PTO. Then the equivalent of something like LLVMPassManagerBuilderSetDisableUnrollLoops() would just modify the PTO.
The legacy PM API separates building the PM and running it on some IR. I’m not sure if we necessarily want to do that when we can build it and run it all at once. But maybe I’m missing a use case.
No, it’s more of an implementation detail that users don’t need to worry about.
I see, I think we’ll omit that parameter from the AfterPassFunc and AfterPassInvalidatedFunc callbacks then.
I’m not following. Isn’t it like all the other things you’re passing into the PassBuilder?
Apologies, that was pretty terribly worded. The PassInstrumentationCallbacks holds all of these " using BeforePassFunc = bool(StringRef, Any);" function types, where the llvm::Any is the IR unit the pass ran over. It could be a llvm::Module, llvm::Function etc… Wouldn’t this be “problematic” for the C interface as we wouldn’t be able to tell what kind of IR unit it was?
Separating it out into a different file from PassBuilder.cpp seems cleanest to me. Maybe right along side it in the same directory? We can always move it around later if we find somewhere better.
That sounds good to me. /llvm/lib/Passes/PassBuilderBindings.cpp maybe?
We should probably do something like heap allocate a PTO/PGOOptions and provide an opaque pointer to it, and set those options, then pass it to the final build/run function. Sort of like LLVMPassManagerRef, but the legacy pass builder stored its options inside the pass builder itself, whereas the new pass builder has a separate PTO. Then the equivalent of something like LLVMPassManagerBuilderSetDisableUnrollLoops() would just modify the PTO.
Yeah I think that’s a nice approach. I believe that’s what the MCJIT bindings do with the LLVMMCJITCompilerOptions struct.
The legacy PM API separates building the PM and running it on some IR. I’m not sure if we necessarily want to do that when we can build it and run it all at once. But maybe I’m missing a use case.
I also thought about that… I think either approach works, and we can always add an API that separates it out or vice versa further down the road if need be.
Let me know what you think. I’ll start working on the API and submit a patch for review soon.
No, it’s more of an implementation detail that users don’t need to worry about.
I see, I think we’ll omit that parameter from the AfterPassFunc and AfterPassInvalidatedFunc callbacks then.
I’m not following. Isn’t it like all the other things you’re passing into the PassBuilder?
Apologies, that was pretty terribly worded. The PassInstrumentationCallbacks holds all of these " using BeforePassFunc = bool(StringRef, Any);" function types, where the llvm::Any is the IR unit the pass ran over. It could be a llvm::Module, llvm::Function etc… Wouldn’t this be “problematic” for the C interface as we wouldn’t be able to tell what kind of IR unit it was?
We wouldn’t be exposing this to the C API right? It’s just another thing to setup right before we create the PassBuilder.
Separating it out into a different file from PassBuilder.cpp seems cleanest to me. Maybe right along side it in the same directory? We can always move it around later if we find somewhere better.
That sounds good to me. /llvm/lib/Passes/PassBuilderBindings.cpp maybe?
Sounds good
We should probably do something like heap allocate a PTO/PGOOptions and provide an opaque pointer to it, and set those options, then pass it to the final build/run function. Sort of like LLVMPassManagerRef, but the legacy pass builder stored its options inside the pass builder itself, whereas the new pass builder has a separate PTO. Then the equivalent of something like LLVMPassManagerBuilderSetDisableUnrollLoops() would just modify the PTO.
Yeah I think that’s a nice approach. I believe that’s what the MCJIT bindings do with the LLVMMCJITCompilerOptions struct.
The legacy PM API separates building the PM and running it on some IR. I’m not sure if we necessarily want to do that when we can build it and run it all at once. But maybe I’m missing a use case.
I also thought about that… I think either approach works, and we can always add an API that separates it out or vice versa further down the road if need be.
Let me know what you think. I’ll start working on the API and submit a patch for review soon.