[llvm] [CodeGen] Fix `MachineModuleInfo`'s move constructor to be more safe with `MCContext` ownership. (PR #104834)

weiwei chen via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 20 09:35:08 PDT 2024


weiweichen wrote:

> > The main issue that's unclear to me for the proposed approach is that who will now be the owner of the `MCContext` if we change the API for `llvm::MachineModuleInfo` to just take a reference to `MCContext`. And if `llvm::MachineModuleInfoWrapperPass` no longer maintain the lifetime of `llvm::MachineModuleInfo`, then who is going to do that?
> 
> Both will have to be explicitly managed by the user of the pipeline, just like the `llvm::LLVMContext` and `llvm::Module` are.
> 
> In MC-related use cases AFAIK (i.e. MC disassembler, MC AsmParsing, LLVM Bolt, etc) `llvm::MCContext` is explicitly created and manged. What I suggested here just extends this to this particular case here.
> 
> As for how MMI will be managed, the new PM driver seems to be initializing and managing it on its own, as it can be seen here in LLC:
> 
> https://github.com/llvm/llvm-project/blob/90a8e5a7ac52d05c2331f0f3d1461fcab6b7d520/llvm/tools/llc/NewPMDriver.cpp#L111
> 
> So I don't think adding an MCContext to the list of explicitly managed things in the pipeline is too much to ask.
> 
> Maybe there is a way to make MMI manage its MCContext without this whole internal/external shenanigans; In that case I see the point of keeping management of the MCContext the way it is in the codegen pipeline.

I don't feel like object to what your intended design/improvement for this part, though that's really not what this PR is for and what the problem I'm trying to solve here. Any chance we can address your design in a separate PR so that I can move on with my downstream project? Happy to review and provide feedbacks on your changes if you'd like to put up a PR to reflect the suggestion!

https://github.com/llvm/llvm-project/pull/104834


More information about the llvm-commits mailing list