Sorry, if this is duplicated subject, but I couldn’t find anything on discourse about that.
What is the current plan for deprecating “-enable-new-pm” option? I see some tests are still using it to disable new PM.
Is it OK, to still use this option and disable new PM, or should we settle only for using default (new) PM?
1 Like
The reason I hadn’t removed it yet is because we still have to support the legacy PM versions of some IR passes because some passes appear in both the optimization pipeline (which uses the new PM) and the codegen pipeline (which uses the legacy PM). Since we still use opt to test codegen-specific IR passes, we need to initialize the legacy PM passes (e.g. initializeScalarOpts in opt.cpp). Initializing legacy PM passes adds the pass name to the global registry and allows people to use the legacy opt syntax opt -instcombine, and without -enable-new-pm it’ll run the legacy PM pass.
But now that I think about it, we don’t have a way to run an entire codegen pipeline (e.g. the X86 -O2 codegen pipeline) from opt, we can only specify a list of IR codegen passes. So actually if we can selectively not initialize optimization IR passes like instcombine (which can be in the codegen pipeline) and only initialize codegen-specific IR passes, the non-codegen specific passes won’t be available as opt -instcombine, only via opt -passes=instcombine, forcing people to only test the new PM pass. And that’s what we want.
We really should remove all -enable-new-pm=0 in tests, then cleanup tests that test optimization passes to use -passes=, then do what I’ve mentioned above and remove the -enable-new-pm.
I’ve been meaning to get around to deleting the legacy PM optimization pipeline, just haven’t found the time to.
Thank you for the clarification and laying down the next steps of the process.
A bit of a wild offer, but maybe you’d be more willing to provide guidance regarding removing it rather than spending your time on this task? This way we can finally remove legacy PM and also I’d be able to gain more experience in new to me parts of LLVM.
Sure!
The first step would be grepping for -enable-new-pm=0 and removing RUN lines for tests that run against both the legacy PM and new PM. If there’s no legacy PM-specific RUN line then trying to make the test work with the new PM would be good.
Also remove tests that test legacy PM-specific passes like pruneeh.
Feel free to ask any followup questions if anything is unclear. Using the New Pass Manager — LLVM 16.0.0git documentation is good if you haven’t already read it.
Ok, thanks!
I’ll start working on this.
I have been looking at PruneEH pass and to be honest, I don’t get how this is only legacy PM-specific pass. I was expecting to see some if condition but to be honest I don’t see anything like that. Which makes it even more difficult to find other similar passes.
Only place where I could find call for PruneEH that made some sense is IPO/PassManagerBuilder.cpp in PassManagerBuilder::populateModulePassManager. But I don’t feel like presence of call to any pass within this function (file?) should indicate that this is legacy PM-specific pass.
Any suggestion, or did I maybe missed something?
PruneEH was intentionally not ported to the new pass manager, ⚙ D44415 [PM][FunctionAttrs] add NoUnwind attribute inference to PostOrderFunctionAttrs pass has some background on that. Basically, function-attrs + simplifycfg should handle everything PruneEH did, plus inlining doesn’t care about calls vs invokes.
So we should remove the PruneEH pass and its tests.
I forgot to mention that we’d need to update bugpoint to work with the new pass manager before removing -enable-new-pm. There was some discussion about this here.
Basically, rather than dealing with a straight pipeline of passes that the legacy pipeline does, we’d need to reduce the nested textual representation of a pipeline. Basically reimplementing reduce_pipeline.py in bugpoint.
Ok, I’ll keep that in mind. For now I still have some tests to update so I’ll start removing legacy PM from compiler sources after that.
Hi!
I have looked into remaining three major groups of tests that are still using legacy PM.
Transforms/PlaceSafepoints (9 tests) - I could only find your commit where you’ve mentioned this pass can be ported to NPM (
D90189 [PlaceSafepoints] Pin tests to legacy PM). Is it still valid approach, or should it be dropped complelty?
Transforms/Inline (10 tests) - as mentioned here D95380 Turn on the new pass manager by default inliner has changed a little with NPM. I’m assuming that in such case either CHECK lines for tests should be updated or tests should be dropped.
Examples/IRTransforms/SimplifyCFG (7 tests) - honestly, in this case I’m not sure what should be the best approach.
PlaceSafepoints is no longer used by anyone to my knowledge, and hasn’t been for a few years. I’m going to post a patch removing it.
For the inliner tests, yes, either updating them or dropping them should be good.
For the Examples/IRTransforms/SimplifyCFG tests, llvm/examples/IRTransforms/SimplifyCFG.cpp should be ported to the new pass manager (there’s even a TODO there for this). We’ll probably need to add to PassRegistry.def something like
#ifdef BUILD_EXAMPLES
FUNCTION_PASS("tut-simplifycfg", TutSimplifyCFGPass)
#endif
Patch to remove PlaceSafepoints is here: ⚙ D135371 Remove PlaceSafepoints pass
Change to remove PlaceSafepoint has landed.
Thanks for update.
Slightly unlelated perhaps. But long time since I read anything about progress about using new-PM for the codegen pipeline.
About -enable-new-pm=0:
I kind of wonder how much testing (using opt) we need to have for those IR passes that are running as part of the codegen pipeline. Historically I think that opt has been used for testing such passes since it is easier to customize the pipeline and run a single pass with opt compared to llc. Today one can use -enable-new-pm=0 to make it possible to verify that the pass still works with legacy PM, so all uses of -enable-new-pm=0 aren’t neccessarily bad (or else one need to design an llc based test to verify how the pass is doing with legacy PM).
About removing legacy PM support in passes:
Removing legacy PM support for an IR pass means that the pass can’t be used in the codegen pipeline any longer. I guess downstream forks would need to deal with it somehow if their backend use such an IR pass in their backend. Or how do we decide which passes that no longer need to support the legacy PM.
About new-pm for codegen pipeline:
I also wonder if there has been any thoughts about splitting the codegen pipeline in two parts. I figure one could use new-PM to run passes up until instructions selection. And then use the legacy PM for the rest of the backend. Here I am assuming that it could work as a smaller step towards removing legacy PM support for all IR passes, while not having to port all the MIR passes to the new PM at the same time.
I believe @aeubanks explanation from one of the first posts is also answering most of your concerns:
But now that I think about it, we don’t have a way to run an entire codegen pipeline (e.g. the X86 -O2 codegen pipeline) from opt, we can only specify a list of IR codegen passes. So actually if we can selectively not initialize optimization IR passes like instcombine (which can be in the codegen pipeline) and only initialize codegen-specific IR passes, the non-codegen specific passes won’t be available as
opt -instcombine, only viaopt -passes=instcombine, forcing people to only test the new PM pass. And that’s what we want.
Sorry if this isn’t the answer you’ve been looking for. I am still learning myself about details related to this effort.
But your question (and mentioned response) actually also made me think about one extra thing related to this change.
@aeubanks does removing of -enable-new-pm=0 also mean removing legacy PM? If not, how will users use legacy PM if everything is supposed to run using new PM?
My plan was to remove legacy PM passes that aren’t being used in any codegen pipelines, removing them from the list of passes that can be run via opt -passname. Legacy PM IR passes in codegen pipelines could still be invoked that way.
The thing that annoys me about this is that we’ll have (e.g. instcombine) tests randomly running against the legacy PM and the new PM depending on what opt syntax people use, because many people adding tests are still most familiar with the legacy syntax. I guess that’s not the worst thing.
If a backend isn’t in tree and they need some IR pass in their codegen pipeline that’s been removed downstream, I’d expect them to revive it downstream.
This thought has occurred to me, but the tradeoff is that any LLVM user has to change their codegen pipeline code to use both the new and legacy pass manager which is annoying. I’m not against this idea, but it’d require some more work in-tree which I don’t currently have time for and would cause some churn.
I see two possible options of porting this pass. Since according to Writing an LLVM Pass — LLVM 18.0.0git documentation pass’s header should be included inside llvm/lib/Passes/PassBuilder.cpp one way can be to move SimplifyCFG.h file to llvm/include/ directory and then update CMakeLists.txt in llvm/examples/IRTransforms/. Another would be to leave header file where it is and update CMakeLists.txt in llvm/lib/Passes/.
Which option is better?
Unless there is third option, much simpler, of which I haven’t thought of. I’d appreciate for any suggestion with that regards.
We should use the pass plugin mechanism for this. For some reason it’s currently documented in the legacy pass documentation: Writing an LLVM Pass — LLVM 16.0.0git documentation. I’ll move it to the new pass manager docs.
There’s an in-tree example here.