[PATCH] D12205: LTO: Simplify merged module ownership.

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Aug-21, at 17:15, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> On Fri, Aug 21, 2015 at 04:50:08PM -0700, Duncan P. N. Exon Smith wrote:
>> Sorry for the delay.  Mehdi and I looked through the code more
>> carefully together.  The call-site for lto_module_dispose() that I
>> found seems to be unreachable when using lto_codegen_set_module().
>> 
>> This means the code generator should be safe to truly take
>> ownership of the module (as the documentation suggested).  If you
>> post an updated patch we can confirm locally that it works.
> 
> Thanks for taking the time to look into this. I have uploaded a new
> version of the patch that restores the ownership semantics from
> my original patch.
> 
>> (Just so it doesn't get lost in all this back and forth: we still
>> need to support ld64's call to lto_codegen_write_merged_modules()
>> that occurs after CodeGen; I think you had a solution in another
>> branch of the thread?)
> 
> Yes, the new patch supports this by removing the call to OwnedModule.reset().
> (The parallel CG solution I mentioned will be posted in a separate patch.)
> 
> Thanks,
> -- 
> Peter

I have some naming/staging nitpicks inline below.  Once I get a chance
to confirm against ld64 (probably not until Monday afternoon, or maybe
Mehdi will beat me to it?) this will LGTM.  Feel free to ping me on
Monday morning.

> Index: lib/LTO/LTOCodeGenerator.cpp
> ===================================================================
> --- lib/LTO/LTOCodeGenerator.cpp
> +++ lib/LTO/LTOCodeGenerator.cpp
> @@ -64,29 +64,20 @@
>  }
>  
>  LTOCodeGenerator::LTOCodeGenerator()
> -    : Context(getGlobalContext()), IRLinker(new Module("ld-temp.o", Context)) {
> +    : Context(getGlobalContext()),
> +      OwnedModule(new Module("ld-temp.o", Context)),

"OwnedContext" made sense as a name because the context isn't always
owned.  We access the context through `Context`, but rely on the
destructor in `OwnedContext` to clean up memory.

Here, we *always* own the module, so we don't need to distinguish.  I
think a clearer name might be "MergedModule", a correctly-capitalized
version of the local variable name used in a few places.

> +      IRLinker(OwnedModule.get()) {
>    initializeLTOPasses();
>  }
>  
>  LTOCodeGenerator::LTOCodeGenerator(std::unique_ptr<LLVMContext> Context)
>      : OwnedContext(std::move(Context)), Context(*OwnedContext),
> -      IRLinker(new Module("ld-temp.o", *OwnedContext)) {
> +      OwnedModule(new Module("ld-temp.o", *OwnedContext)),
> +      IRLinker(OwnedModule.get()) {
>    initializeLTOPasses();
>  }
>  
> -void LTOCodeGenerator::destroyMergedModule() {
> -  if (OwnedModule) {
> -    assert(IRLinker.getModule() == &OwnedModule->getModule() &&
> -           "The linker's module should be the same as the owned module");
> -    delete OwnedModule;
> -    OwnedModule = nullptr;
> -  } else if (IRLinker.getModule())
> -    IRLinker.deleteModule();
> -}
> -
> -LTOCodeGenerator::~LTOCodeGenerator() {
> -  destroyMergedModule();
> -}
> +LTOCodeGenerator::~LTOCodeGenerator() {}
>  
>  // Initialize LTO passes. Please keep this funciton in sync with
>  // PassManagerBuilder::populateLTOPassManager(), and make sure all LTO
> @@ -408,7 +397,6 @@
>  void LTOCodeGenerator::applyScopeRestrictions() {
>    if (ScopeRestrictionsDone || !ShouldInternalize)
>      return;
> -  Module *mergedModule = IRLinker.getModule();

If you rename this variable in a separate NFC commit to 'MergedModule'
it would minimize noise in the semantic change.

>  
>    // Start off with a verification pass.
>    legacy::PassManager passes;
> @@ -422,20 +410,17 @@
>    TargetLibraryInfoImpl TLII(Triple(TargetMach->getTargetTriple()));
>    TargetLibraryInfo TLI(TLII);
>  
> -  accumulateAndSortLibcalls(Libcalls, TLI, *mergedModule, *TargetMach);
> +  accumulateAndSortLibcalls(Libcalls, TLI, *OwnedModule, *TargetMach);
>  
> -  for (Module::iterator f = mergedModule->begin(),
> -         e = mergedModule->end(); f != e; ++f)
> -    applyRestriction(*f, Libcalls, MustPreserveList, AsmUsed, Mangler);

I think switching to range-based fors should also be a separate
NFC commit (could certainly be merged with the name change).

> -  for (Module::global_iterator v = mergedModule->global_begin(),
> -         e = mergedModule->global_end(); v !=  e; ++v)
> -    applyRestriction(*v, Libcalls, MustPreserveList, AsmUsed, Mangler);
> -  for (Module::alias_iterator a = mergedModule->alias_begin(),
> -         e = mergedModule->alias_end(); a != e; ++a)
> -    applyRestriction(*a, Libcalls, MustPreserveList, AsmUsed, Mangler);
> +  for (Function &f : *OwnedModule)
> +    applyRestriction(f, Libcalls, MustPreserveList, AsmUsed, Mangler);
> +  for (GlobalVariable &v : OwnedModule->globals())
> +    applyRestriction(v, Libcalls, MustPreserveList, AsmUsed, Mangler);
> +  for (GlobalAlias &a : OwnedModule->aliases())
> +    applyRestriction(a, Libcalls, MustPreserveList, AsmUsed, Mangler);
>  
>    GlobalVariable *LLVMCompilerUsed =
> -    mergedModule->getGlobalVariable("llvm.compiler.used");
> +    OwnedModule->getGlobalVariable("llvm.compiler.used");
>    findUsedValues(LLVMCompilerUsed, AsmUsed);
>    if (LLVMCompilerUsed)
>      LLVMCompilerUsed->eraseFromParent();
> @@ -474,16 +459,14 @@
>    if (!this->determineTarget(errMsg))
>      return false;
>  
> -  Module *mergedModule = IRLinker.getModule();
> -

Same trick as above for the name change.

>    // Mark which symbols can not be internalized
>    this->applyScopeRestrictions();
>  
>    // Instantiate the pass manager to organize the passes.
>    legacy::PassManager passes;
>  
>    // Add an appropriate DataLayout instance for this module...
> -  mergedModule->setDataLayout(TargetMach->createDataLayout());
> +  OwnedModule->setDataLayout(TargetMach->createDataLayout());
>  
>    passes.add(
>        createTargetTransformInfoWrapperPass(TargetMach->getTargetIRAnalysis()));
> @@ -513,8 +496,6 @@
>    if (!this->determineTarget(errMsg))
>      return false;
>  
> -  Module *mergedModule = IRLinker.getModule();
> -

Same trick here too.

>    legacy::PassManager codeGenPasses;
>  
>    // If the bitcode files contain ARC code and were compiled with optimization,