Duncan P. N. Exon Smith
2014-Jan-22 01:39 UTC
[LLVMdev] [RFC] LTO: deallocating llvm::Module inside lto_codegen_add_module
LTOCodeGenerator::addModule (which is wrapped by lto_codegen_add_module) takes an LTOModule as an argument and links the latter's llvm::Module into its own. It does not change the latter's llvm::Module. The lto_module_dispose API call destroys an LTOModule. This frees the LTOModule's llvm::Module, but also invalidates strings returned by, e.g., lto_module_get_symbol_name. I propose that LTOCodeGenerator::addModule be changed to also deallocate the (consumed and redundant) llvm::Module and llvm::TargetMachine objects left behind in LTOModule. I.e., something akin to: diff --git a/include/llvm/LTO/LTOModule.h b/include/llvm/LTO/LTOModule.h index c70afa4..1180e58 100644 --- a/include/llvm/LTO/LTOModule.h +++ b/include/llvm/LTO/LTOModule.h @@ -164,6 +164,11 @@ public: return _asm_undefines; } + void destroyLLVMModule() { + _module.reset(); + _target.reset(); + } + private: /// parseMetadata - Parse metadata from the module // FIXME: it only parses "Linker Options" metadata at the moment diff --git a/lib/LTO/LTOCodeGenerator.cpp b/lib/LTO/LTOCodeGenerator.cpp index cae0ea2..a947fa8 100644 --- a/lib/LTO/LTOCodeGenerator.cpp +++ b/lib/LTO/LTOCodeGenerator.cpp @@ -121,6 +121,8 @@ bool LTOCodeGenerator::addModule(LTOModule* mod, std::string& errMsg) { for (int i = 0, e = undefs.size(); i != e; ++i) AsmUndefinedRefs[undefs[i]] = 1; + mod->destroyLLVMModule(); + return !ret; } With this change, the ownership of the llvm::Module and llvm::TargetMachine is being transferred from the LTOModule to the LTOCodeGenerator. Practically, this would decouple the deallocation of the main contents of LTOModule from the deallocation of its strings that have been passed to the linker (e.g., ld64). It would also give a memory win for consumers that do not immediately call lto_module_dispose, without requiring a change to their code. However, any consumers of the LTO interface that rely on the llvm::Module and/or llvm::TargetMachine being accessible *after* LTOCodeGenerator::addModule would be in trouble. A few questions for comment: 1. Is this considered a change to the C API (and, thus, banned)? 2. Are there any consumers that rely on llvm::Module being accessible after LTOCodeGenerator::addModule? What about llvm::TargetMachine? 3. Would the proposed behaviour be especially surprising?
Chandler Carruth
2014-Jan-22 02:38 UTC
[LLVMdev] [RFC] LTO: deallocating llvm::Module inside lto_codegen_add_module
My 2 cents... On Tue, Jan 21, 2014 at 5:39 PM, Duncan P. N. Exon Smith < dexonsmith at apple.com> wrote:> 1. Is this considered a change to the C API (and, thus, banned)? >I think this would indeed be a significant semantic change to the C API.> 2. Are there any consumers that rely on llvm::Module being accessible after > LTOCodeGenerator::addModule? What about llvm::TargetMachine? >I think we have to assume so. That's the down side to saying it is a stable C API.> 3. Would the proposed behaviour be especially surprising? >No, the behavior you describe seems sensible. I would just expect it to need to go under a separate/new API to preserve backwards compatibility. -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20140121/9df8b202/attachment.html>
Duncan P. N. Exon Smith
2014-Jan-22 22:05 UTC
[LLVMdev] [RFC] LTO: deallocating llvm::Module inside lto_codegen_add_module
On Jan 21, 2014, at 6:38 PM, Chandler Carruth <chandlerc at google.com> wrote:> My 2 cents... > > On Tue, Jan 21, 2014 at 5:39 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > 1. Is this considered a change to the C API (and, thus, banned)? > > I think this would indeed be a significant semantic change to the C API. > > 2. Are there any consumers that rely on llvm::Module being accessible after > LTOCodeGenerator::addModule? What about llvm::TargetMachine? > > I think we have to assume so. That's the down side to saying it is a stable C API. > > 3. Would the proposed behaviour be especially surprising? > > No, the behavior you describe seems sensible. I would just expect it to need to go under a separate/new API to preserve backwards compatibility.Thanks Chandler. I’ll go ahead with new API.