[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