> On 2015-Aug-20, at 22:04, Peter Collingbourne <peter at pcc.me.uk> wrote:
>
> On Thu, Aug 20, 2015 at 05:25:32PM -0700, Duncan P. N. Exon Smith wrote:
>>> --- tools/llvm-lto/llvm-lto.cpp
>>> +++ tools/llvm-lto/llvm-lto.cpp
>>> @@ -207,26 +207,24 @@
>>> return 1;
>>> }
>>>
>>> - LTOModule *LTOMod = Module.get();
>>> -
>>> - // We use the first input module as the destination module when
>>> - // SetMergedModule is true.
>>> - if (SetMergedModule && i == BaseArg) {
>>> - // Transfer ownership to the code generator.
>>> - CodeGen.setModule(Module.release());
>>> - } else if (!CodeGen.addModule(Module.get()))
>>> - return 1;
>>> -
>>> - unsigned NumSyms = LTOMod->getSymbolCount();
>>> + unsigned NumSyms = Module->getSymbolCount();
>>> for (unsigned I = 0; I < NumSyms; ++I) {
>>> - StringRef Name = LTOMod->getSymbolName(I);
>>> + StringRef Name = Module->getSymbolName(I);
>>> if (!DSOSymbolsSet.count(Name))
>>> continue;
>>> - lto_symbol_attributes Attrs = LTOMod->getSymbolAttributes(I);
>>> + lto_symbol_attributes Attrs = Module->getSymbolAttributes(I);
>>> unsigned Scope = Attrs & LTO_SYMBOL_SCOPE_MASK;
>>> if (Scope != LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN)
>>> KeptDSOSyms.push_back(Name);
>>> }
>>> +
>>> + // We use the first input module as the destination module when
>>> + // SetMergedModule is true.
>>> + if (SetMergedModule && i == BaseArg) {
>>> + // Transfer ownership to the code generator.
>>> + CodeGen.setModule(std::move(Module));
>>> + } else if (!CodeGen.addModule(Module.get()))
>>> + return 1;
>>
>> Why have you moved this code?
>
> This was needed because of the ownership change -- see below.
Might as well split it out.
>>> LTOCodeGenerator::~LTOCodeGenerator() {
>>> - destroyMergedModule();
>>> -
>>
>> Could the ordering between tearing down the `LTOModule` and the
>> `TargetMach` matter?
>
> From a quick scan I couldn't see anything inside any derived class of
> TargetMachine that preserves a reference to anything inside the Module,
> so I don't think it should matter.
I was thinking about the other direction, since the TargetMachine was
being deleted second (and your patch changed the order). However there
clearly aren't any references from Module to TargetMachine (maybe I had
DataLayout in mind or something??).
>> I suggest moving `TargetMach` to a
>> `std::unique_ptr<>` first in a separate commit (looks like it would be
>> easy?), and placing it such that the ordering won't change.
>
> r245670. (Note that this wouldn't change the order anyway, as the members
> would be destroyed after the destructor is called.)
(Once you make them both unique_ptrs, the ordering depends on the order
of the members.)
>> Seems like you could through the CodegenOptions into a unique_ptr as
>> well (using a custom deleter that calls `free()`). Might be nice to
>> clean that up as well (also in a separate commit).
>
> Or I could use std::vector<std::string> and build the char * vector
> on demand, which seems like the simplest approach. r245671.
I assumed pointer stability mattered, but it looks like it doesn't.
Nice.
>
>>> +
>>> + // Finally destroy the module as we no longer need it
>>> + OwnedModule.reset();
>>
>> Yes we do! API users can call `lto_codegen_write_merged_modules()`
>> after this.
>
> So they can. Hrm.
>
> So the problem I have is that in order to implement parallel LTO code
> generation efficiently, I would like all code generation in this class (and
> the gold plugin) to go through a function whose signature is something like
>
> void codegen(std::unique_ptr<Module> M, std::vector<ostream> outputs, x foo, y bar, z baz, ...);
>
> so that the codegen function is free to mangle M arbitrarily then
> destroy it. (This would be useful if, say, the work is partitioned
> among multiple modules; M could be reused by stripping it of all but the
> contents of a single partition.) This would prevent clients from calling
> lto_codegen_write_merged_modules() and getting what they want even if we
> did change the function to not take ownership of the module.
>
> I suppose that I could make this function's signature look like this:
>
> std::unique_ptr<Module> codegen(std::unique_ptr<Module> M, std::vector<ostream> outputs, x foo, y bar, z baz, ...);
>
> where the result is equal to M if the parallelism level is 1, or
> std::unique_ptr<Module>() otherwise. The code in LTOCodeGenerator
> would then look like this:
>
> OwnedModule = codegen(std::move(OwnedModule), ...);
>
> This would mean that clients would need to change to doing something like
> this if they want to save IR before parallel CG:
>
> lto_codegen_set_module();
> lto_codegen_add_module();
> lto_codegen_add_module();
> lto_codegen_optimize();
> lto_codegen_write_merged_modules();
> lto_codegen_compile_optimized();
>
> but the semantics for non-parallel CG are preserved.
FWIW, this is a better place to be writing out IR anyway. The early
CodeGen passes mutate the IR in ways that make it hard to correctly run
CodeGen again (some types of lowering can't be run twice). I'd
encourage all clients to write out here instead of after CodeGen (Manman
split apart optimize and compile_optimized in order to enable this flow,
primarily so that the output of `ld64 -save-temps` can be run through
`llc` directly).
All that to say, I think this new flow makes sense: if some client
decides to use the parallel options, then they need to adopt the new
flow for writing out IR.
>
>>
>>>
>>> return true;
>>> }
>>> Index: include/llvm/Linker/Linker.h
>>> ===================================================================
>>> --- include/llvm/Linker/Linker.h
>>> +++ include/llvm/Linker/Linker.h
>>> @@ -62,7 +62,6 @@
>>>
>>> Linker(Module *M, DiagnosticHandlerFunction DiagnosticHandler);
>>> Linker(Module *M);
>>> - ~Linker();
>>
>> This seems unrelated?
>
> r245672.
>
>>> --- include/llvm-c/lto.h
>>> +++ include/llvm-c/lto.h
>>> @@ -40,7 +40,7 @@
>>> * @{
>>> */
>>>
>>> -#define LTO_API_VERSION 17
>>> +#define LTO_API_VERSION 18
>>
>> There's no new API, so the version shouldn't be changing.
>>
>>>
>>> /**
>>> * \since prior to LTO_API_VERSION=3
>>> @@ -374,8 +374,7 @@
>>> lto_codegen_add_module(lto_code_gen_t cg, lto_module_t mod);
>>>
>>> /**
>>> - * Sets the object module for code generation. This will transfer the ownship of
>>> - * the module to code generator.
>>> + * Sets the object module for code generation. This will destroy the module.
>>
>> Was this poorly documented, or is your patch intended to change the
>> semantics of this call? The latter would be a problem.
>
> The latter. How important is it that we preserve these semantics? It looks
> like this function was introduced by Manman in r230290, so presumably this
> is being used by some version of ld64, but I cannot see any uses of this
> function in ld64-242.
It's used in the version of ld64 that's in the Xcode 7 betas. Changing
the semantics in this way would crash, since ld64 later calls
lto_module_dispose().
> How difficult would it be to change ld64 to not rely on the module staying
> around? There would be no real ownership changes as such, so I think all
> you would need to do is make sure that the linker does not touch the module
> after calling lto_codegen_set_module.
Unfortunately, it's fairly baked at this point :(.
> The only other linker I am aware of that uses libLTO is Sony's. Yunzhong,
> would this change affect you significantly?
>
> There are other things we can do if we absolutely have to preseve these
> semantics, but it would add complexity to LTOCodeGenerator.
If you use a std::shared_ptr, you can assign a null deleter when the
source of the module is `lto_codegen_set_module()`. Does that help?