[ThinLTO] Launch importing backends in parallel threads from gold plugin

[tejohnson]

Summary

Instead of exiting after creating the combined index in the gold plugin,
unless requested by new option, we will now launch the ThinLTO backends
(LTO and codegen pipelines with importing) in parallel threads. The
number of threads is controlled by the existing -jobs gold plugin option,
or the hardware concurrency if not specified.

As discussed on IRC with Rafael, pull split codegen into gold-plugin and
use the ThreadPool support. Refactor both the split codegen and ThinLTO
handling to utilize a new CodeGen class that encapsulates the
optimization and code generation handling for each module (split or
not). This allows better code reuse between the ThinLTO and split
codegen cases. For now I have included this along with the ThinLTO thread patch, to
show how it all fits together. I can commit the split code gen changes
first though, followed by the ThinLTO backend support. Let me know if
you would like to review these separately.

Along with follow-on fixes D16173 and D16120, all of the SPEC cpu2006 C/C++ benchmarks now build and run correctly with -flto=thin.

[tejohnson]

Rebase to pick up changes I committed separately. The changes in this patch are now just related to the ThinLTO thread support.

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

272 ↗

(On Diff #42323)

Const correctness is great, please commit now separately.

tools/gold/gold-plugin.cpp

940 ↗

(On Diff #42323)

We should probably refactor this out of the plugin, but this can be done later.

979 ↗

(On Diff #42323)

This is very suboptimal, I don't mind if you want to get this in for now as it is limited to the gold plugin.

I plan to submit a ThreadPool to LLVMSupport (I'm using it locally in my bringup of the ld64 plugin).

[tejohnson]

Thanks for the comments.

lib/Transforms/IPO/FunctionImport.cpp

272 ↗

(On Diff #42323)

Done already, just rebased this to pick it up.

tools/gold/gold-plugin.cpp

940 ↗

(On Diff #42323)

Right, I am wondering how much overlap there is with the support you were adding for ld64. Would putting it in libLTO be useful?

979 ↗

(On Diff #42323)

Right, part of the reason I wanted to send this right away was to see if there was something existing or under development so that I didn't have to reinvent the wheel. Glad to hear you had a similar need for this. Do you expect it to go in soon?

[tejohnson]

Biggest change in this update is due to gold not being thread-safe
by default, requiring lots of refactoring to make gold callbacks in
single-threaded mode.

Also changed from std::thread to the thread wrapper in LLVM, which
handles !LLVM_ENABLE_THREADS.

Some test updates to test both single and multi-threaded handling.

I have not refactored any of the code out of gold yet. Doing so will
require refactoring out other routines, such as codegenImpl and
its callees such as runLTOPasses, or invoking via a callback.
There is some overlap between the handling in these routines and
handling that exists currently in LTOCodeGenerator, which we could
refactor out of both. I'm not sure where the best place to put the
refactored code is, maybe lib/CodeGen (which is where splitCodeGen lives)?

[tejohnson]

In D15390#308252, @tejohnson wrote:

Biggest change in this update is due to gold not being thread-safe
by default, requiring lots of refactoring to make gold callbacks in
single-threaded mode.

Also changed from std::thread to the thread wrapper in LLVM, which
handles !LLVM_ENABLE_THREADS.

Some test updates to test both single and multi-threaded handling.

I have not refactored any of the code out of gold yet. Doing so will
require refactoring out other routines, such as codegenImpl and
its callees such as runLTOPasses, or invoking via a callback.
There is some overlap between the handling in these routines and
handling that exists currently in LTOCodeGenerator, which we could
refactor out of both. I'm not sure where the best place to put the
refactored code is, maybe lib/CodeGen (which is where splitCodeGen lives)?

It is more than just codegenImpl, but also getModuleForFile that would have to be refactored out of gold. Right now looking at the ThinLTO thread handling in gold as well as the LTO pass and codegen invocations in libLTO (LTOCodeGenerator), I don't think we gain much by refactoring this out. The actual thread handling in gold is pretty minimal, and very specific to gold. So I'd prefer to keep this in the gold plugin at least for now.

[tejohnson]

No longer WIP. Tested with both regression tests and several SPEC cpu2006 benchmarks. PTAL.

[tejohnson]

• Rebase and change to use new ThreadPool support.

[• rafael]

test/tools/gold/X86/thinlto.ll

12 ↗

(On Diff #42860)

This gold invocation is not being tested.

32 ↗

(On Diff #42860)

These two only check the t4.thinlto.bc. Don't you want to, for example, run llvm-nm on t4?

tools/gold/gold-plugin.cpp

33 ↗

(On Diff #42860)

Why do you need Linker.h?

[tejohnson]

test/tools/gold/X86/thinlto.ll

12 ↗

(On Diff #42860)

Right, it was just checking to ensure that it succeeded without an error.

32 ↗

(On Diff #42860)

Same as above, it was just checking for the invocation succeeding without an error. I could run llvm-nm on the output file and check for "T f", just to make sure it is there and not ill-formed. Is that what you had in mind?

tools/gold/gold-plugin.cpp

33 ↗

(On Diff #42860)

renameModuleForThinLTO

[tejohnson]

• Add check for expected gold output file.

[• rafael]

tools/gold/gold-plugin.cpp

80 ↗

(On Diff #42870)

thread or task now?

920 ↗

(On Diff #42870)

Start the function name with a lower case.

This is not a thread anymore. Task maybe?

925 ↗

(On Diff #42870)

It is thread safe since you create a new one for each thread, no?

940 ↗

(On Diff #42870)

Don't we want something that uses gold's symbol resolution?

renameModuleForThinLTO will copy even stuff that gold has marked as preempted, no?

971 ↗

(On Diff #42870)

This seems incompatible with threading or even multiple outputs, no? It looks like every thread will try to use the same output file name.

I would suggest producing an error for now.

[tejohnson]

Thanks for the comments. Responses and a couple questions below.

tools/gold/gold-plugin.cpp

80 ↗

(On Diff #42870)

Yeah, will change to TaskInfo and update the comments accordingly.

920 ↗

(On Diff #42870)

Will fix both issues

925 ↗

(On Diff #42870)

The comment was not entirely clear - what I meant was that I am creating a new one for each thread/task because of the fact that it is not thread-safe (i.e. they can't all share the same context). Will clarify.

940 ↗

(On Diff #42870)

Unfortunately all the ThinLTO promotion logic and renaming support is in the ModuleLinker, so I couldn't just use IRMover::move with the Keep list.

Perhaps the ModuleLinker should have a mode where all it does is the promotion handling for exporting modules before calling IRMover::move. I.e. I think this would just need to do a version of processGlobalsForThinLTO where locals in a supplied ValuesToLink list (initiallized from the Keep list in gold) would be promoted if necessary. Similar to the existing processGlobalsForThinLTO but only for things already in the supplied ValuesToLink.

Does that sound right?

971 ↗

(On Diff #42870)

The openOutputFile helper below will append a unique thread ID (should probably change this to TaskID...). Will add comment to that effect. Or do you still think it is better to error?

[• rafael]

Comment at: tools/gold/gold-plugin.cpp:940
@@ +939,3 @@
+ std::unique_ptr<llvm::Module> RenamedModule =
+ renameModuleForThinLTO(M, &CombinedIndex);

+ if (!RenamedModule)

rafael wrote:

Don't we want something that uses gold's symbol resolution?

renameModuleForThinLTO will copy even stuff that gold has marked as preempted, no?

Unfortunately all the ThinLTO promotion logic and renaming support is in the ModuleLinker, so I couldn't just use IRMover::move with the Keep list.

Perhaps the ModuleLinker should have a mode where all it does is the promotion handling for exporting modules before calling IRMover::move. I.e. I think this would just need to do a version of processGlobalsForThinLTO where locals in a supplied ValuesToLink list (initiallized from the Keep list in gold) would be promoted if necessary. Similar to the existing processGlobalsForThinLTO but only for things already in the supplied ValuesToLink.

Does that sound right?

Not sure. Thinking a bit more about it I think I am missing the big picture.

I was at least under the impression that we could:

• Run the IRMover more or less like we do in normal LTO, but instead

of moving to a merged module, each task gets one file and moves it
into an empty module.

• Run a transformation that updates name and visibility in place.
• For each module we want to cherry pick something, FunctionImport brings it in.

Comment at: tools/gold/gold-plugin.cpp:971
@@ +970,3 @@
+ if (!options::obj_path.empty())
+ Filename = options::obj_path;

+ else if (options::TheOutputType == options::OT_SAVE_TEMPS)

rafael wrote:

This seems incompatible with threading or even multiple outputs, no? It looks like every thread will try to use the same output file name.

I would suggest producing an error for now.

The openOutputFile helper below will append a unique thread ID (should probably change this to TaskID...). Will add comment to that effect. Or do you still think it is better to error?

No, but please add a test :-)

Cheers,
Rafael

[tejohnson]

• Rebase and address Rafael's review comments.
  
  I think I have addressed all your comments. Notable changes from prior version:
• Use gold's symbol resolution via IRMover, and invoke renameModuleForThinLTO afterwards to do renaming (with TODO noting that this is temporary until we can do this in place)
• Rebase to use new RAII wrapper for plugin file handling. Add move assignment/copy constructor to enable moving ownership into TaskInfo object. Change RAII PluginInputFile wrapper to use a unique_ptr for the ld_plugin_input_file object so that it can be moved, and add a flag to prevent double-release on a moved object.
• s/Thread/Task/
• Add test to ensure gold's symbol resolution not overridden.
• Add test for plugin option obj-path handling with ThinLTO threads

[• rafael]

tools/gold/gold-plugin.cpp

81 ↗

(On Diff #43060)

Please rebase the patch.

82 ↗

(On Diff #43060)

You can just memcpy the ld_plugin_input_file, no ?

[tejohnson]

tools/gold/gold-plugin.cpp

81 ↗

(On Diff #43060)

Will do and upload the new one shortly.

82 ↗

(On Diff #43060)

I could and I considered that. As currently defined by gold it would save to memcpy. However, I thought it would be better to use a unique_ptr since it doesn't assume anything about the structure which isn't defined here, and it seemed clearer and cleaner to avoid copying. Note we pass a reference to this member to the ThreadPool::async to be used by the thread, that would have to be changed to a memcpy as well.

[tejohnson]

Rebased

[• rafael]

tools/gold/gold-plugin.cpp

83 ↗

(On Diff #43165)

OK. If we have a std::unique_ptr, we can use it instead of the Valid field, no? Valid is false iff File is null.

[• rafael]

I got this warning:

/home/espindola/llvm/llvm/tools/gold/gold-plugin.cpp:133:17: warning: private field 'F' is not used [-Wunused-private-field]

claimed_file *F

tools/gold/gold-plugin.cpp

979 ↗

(On Diff #43165)

If you change codegenImpl to take an ArrayRef you don't have to do this.

[tejohnson]

In D15390#313349, @rafael wrote:

I got this warning:

/home/espindola/llvm/llvm/tools/gold/gold-plugin.cpp:133:17: warning: private field 'F' is not used [-Wunused-private-field]

claimed_file *F

Will fix. Looks like there are some stale comments about using this class for the join, which isn't necessary after switching to the ThreadPool. Will clean that up.

tools/gold/gold-plugin.cpp

83 ↗

(On Diff #43165)

Good point, will fix this.

979 ↗

(On Diff #43165)

Ok, will change.

[• rafael]

We should probably refactor splitCodeGen. It is odd that now we have
two parallel codegen paths. With thinLTO we already multiple BC files,
so it should probably look something like

if (SplitForParallelCodeGen)

ProduceMultipleModules();

Create the tasks.

Each task handles one bc file, which may be one of the original ones
if using thinLto or one of the split ones.

[tejohnson]

• Address review comments/suggestions

[tejohnson]

In D15390#313367, @rafael wrote:

We should probably refactor splitCodeGen. It is odd that now we have
two parallel codegen paths. With thinLTO we already multiple BC files,
so it should probably look something like

if (SplitForParallelCodeGen)

ProduceMultipleModules();

Create the tasks.

Each task handles one bc file, which may be one of the original ones
if using thinLto or one of the split ones.

This will require some refactoring of SplitModule() as well, which currently takes a callback (that actually creates each thread) and does the module splitting. For the case where we don't want multiple split modules, like in ThinLTO, we simply pass a single output stream. Note that in both the split and non-split case the same codegen() routine is called to do the actual codegen part.

I think I've addressed all of your other comments. PTAL. Thanks!

[tejohnson]

Ping

[• rafael]

tools/gold/gold-plugin.cpp

891 ↗

(On Diff #43184)

splitCodeGen can take a ArrayRef too. Why do you need the vec?

975 ↗

(On Diff #43184)

It seems odd how much work the destructor of TaskInfo is doing.

Most of the work is here because gold is not thread safe, correct? It so, it seems better to write this code explicitly after

ThinLTOThreadPool.wait();

998 ↗

(On Diff #43184)

Why do you need a worklist?

Can't you just just a simple loop over Modules?

1033 ↗

(On Diff #43184)

This can be just

Tasks.emplace_back(new TaskInfo(std::move(InputFile), std::move(OS),
                                NewFilename.c_str(), TempOutFile));

[tejohnson]

Per IRC discussion, will do some refactoring of splitCodeGen next, then subsequently rebase this patch on top of that. But I wanted to reply to the latest comments here and upload a new patch that addresses them first.

tools/gold/gold-plugin.cpp

891 ↗

(On Diff #43184)

Ah ok, fixed.

975 ↗

(On Diff #43184)

Changed TaskInfo::~TaskInfo into TaskInfo::cleanup and invoked explicitly on each task after the wait().

998 ↗

(On Diff #43184)

I think this was leftover from my original pre-ThreadPool implementation. Good point that it isn't needed. Updated to iterate over Modules as suggested.

1033 ↗

(On Diff #43184)

Fixed

[tejohnson]

• Address latest feedback.

[tejohnson]

As discussed on IRC with Rafael, pull split codegen into gold-plugin and
use the ThreadPool support. Refactor both the split codegen and ThinLTO
handling to utilize a new CodeGen class that encapsulates the
optimization and code generation handling for each module (split or
not). This allows better code reuse between the ThinLTO and split
codegen cases.

For now I have included this along with the ThinLTO thread patch, to
show how it all fits together. I can commit the split code gen changes
first though, followed by the ThinLTO backend support. Let me know if
you would like to review these separately.

[mehdi_amini]

include/llvm/Support/thread.h

60 ↗

(On Diff #43184)

Can be committed separately I think.

[tejohnson]

Ping.

include/llvm/Support/thread.h

60 ↗

(On Diff #43745)

Will do.

[tejohnson]

• Rebase and improve -save-temps behavior with ThinLTO

[tejohnson]

Ping.

Using this support extensively in my own ThinLTO spec testing. Would be great to get this reviewed and in tree. =)

Note it involves some refactoring of the split codegen path as suggested by Rafael on IRC (see the comment history for details, specifically Dec 29 update).

[mehdi_amini]

I'm not familiar with Gold, but here are a few minor comments

tools/gold/gold-plugin.cpp

102 ↗

(On Diff #44824)

Any difference with PluginInputFile(PluginInputFile &&RHS) = default; ?

107 ↗

(On Diff #44824)

(same =default here)

826 ↗

(On Diff #44824)

Note: you could reuse the TargetMachine for the next module processed by this thread.

1104 ↗

(On Diff #44824)

There is a bunch of duplicated code above (used in regular LTO as well I think)

[tejohnson]

In D15390#338559, @joker.eph wrote:

I'm not familiar with Gold, but here are a few minor comments

Thanks for the comments!

tools/gold/gold-plugin.cpp

102 ↗

(On Diff #44824)

Good point, will change to default

107 ↗

(On Diff #44824)

Ditto.

826 ↗

(On Diff #44824)

That's an interesting idea. But I don't think I have any ability to control this once I send tasks to the thread pool. Is there a good way to share things across tasks assigned to the same thread by the pool?

1104 ↗

(On Diff #44824)

True, the LTO handling in allSymbolsReadHook does some of the same things. But the LLVMContext and IRMover constructors are outside the loop over the modules since they can be shared in that case. And the invocation of getModuleForFile is a bit different. I could probably create a helper that does the getModuleForFile, setting of the target triple, and invoke IRMover::move though. I'm not sure if that ends up being clearer, but let me see what I could do here.

[tejohnson]

Address review comments. Use default move constructors, and refactor
common code into a helper.

[mehdi_amini]

Some more comment.

tools/gold/gold-plugin.cpp

805 ↗

(On Diff #46408)

It took me some time to understand what was going on, this "recursive" use of the CodeGen class can be confusing. As long as it is limited to this file I won't object.

820 ↗

(On Diff #46408)

Yeah is it annoying, in my local implementation I store a "per thread context" in a global map protect retrieving the Context with a mutex.
(I'm not asking you to do the same here and now)

1043 ↗

(On Diff #46408)

Usually I prefer RAII (i.e. using a new scope).

1121 ↗

(On Diff #46408)

This could be

std::vector<ThinLTOTaskInfo> Tasks;
Tasks.reserve(Modules.size());

(same above for std::vector<std::unique_ptr<TaskInfo>> Tasks; around line 1023)

1169 ↗

(On Diff #46408)

Could this be done in the TaskInfo dtor?

[tejohnson]

tools/gold/gold-plugin.cpp

805 ↗

(On Diff #46408)

Yeah, it was unfortunatly hard to get the refactoring and code sharing between the different modes without doing this. So I tried to document it as well as I could.

1043 ↗

(On Diff #46408)

Oh I see, the wait() is unnecessary if I provoke the ThreadPool destructor via RAII. Will do that here and for the ThinLTO thread pool as well.

1121 ↗

(On Diff #46408)

Ok

1169 ↗

(On Diff #46408)

I previously had it there, but Rafael thought the dtor was too heavy-weight and wanted it more explicit. =)

[tejohnson]

Address more review comments: Use RAII on ThreadPool instead of expicit
wait(), and reserve TaskInfo vectors rather than emplacing unique_ptrs.

[tejohnson]

Ping

[• rafael]

tools/gold/gold-plugin.cpp

972 ↗

(On Diff #46593)

s/thread/task/

1029 ↗

(On Diff #46593)

It is nice that now we always use a ThreadPool.

It would be awesome if this could be refactored so that there was just one ThreadPool for thinlto and conventional parallel codegen.

[tejohnson]

tools/gold/gold-plugin.cpp

972 ↗

(On Diff #46593)

Fixed here and a couple other places.

1029 ↗

(On Diff #46593)

The task type is different, and the iteration/handling to add tasks is different - I'm not sure how much code reuse we would get by sharing the thread pool creation and management code. The code to create the thread pool and insert into it is pretty minimal by itself. Also note that you never are using both thread pools in a single compilation.

[tejohnson]

• s/thread/task/ in a couple places

[pcc]

tools/gold/gold-plugin.cpp

1019 ↗

(On Diff #47458)

I don't think thread pools are necessary for split code gen, as we can already perfectly assign the right amount of work to individual threads. Also, this implementation loses the pipelining feature from the original code (i.e. worker threads can work on codegen'ing while the main thread is still splitting). I would prefer you to use the existing implementation in llvm/CodeGen/ParallelCG.h.

[mehdi_amini]

tools/gold/gold-plugin.cpp

1019 ↗

(On Diff #47458)

The thread that is doing the splitting can issue other jobs to the thread pool, providing the desired pipeline.

Just fuse the loop body below within the lambda...

[mehdi_amini]

tools/gold/gold-plugin.cpp

1019 ↗

(On Diff #47458)

I'll add that while the pooling is not necessary if you only queue as many jobs as you have threads, it is not a reason by itself not to use it: the paradigm is fairly clear, and it decouples the actual splitting granularity from the number of actual worker threads, allowing to experiment with different numbers for each (providing better pipelining for instance).

[pcc]

tools/gold/gold-plugin.cpp

1019 ↗

(On Diff #47458)

Yes, but the current implementation doesn't need any of that. If we experimentally find that decoupling would provide some benefit, then by all means we can start using thread pools here.

In any case, if there is a compelling reason to use thread pools, the right place to make the change is in lib/CodeGen/ParallelCG.cpp rather than in a duplicate implementation here. We can defer what the design for that should look like simply by not using thread pools yet.

[tejohnson]

Modify the patch to implement what is hopefully a compromise solution on
split code gen. I modified lib/CodeGen/ParallelCG.cpp to use a
ThreadPool, and go back to invoking it from the gold plugin.

This has a few nice effects:

• ThreadPool used by all ParallelCG consumers.
• Restores the pipelining of splitting and codegen (although note that with a tweak to the old version of this thread that this could be attained in the gold-plugin implementation as well).
• Avoids the recursive construction of the CodeGen object on the split code gen path.

Can one of you take a look and see if this is acceptable, and if so and
there are not other comments, mark it accepted?

[tejohnson]

Update a comment to match new version. Also, rename the CodeGen Filename
member to SaveTempsFilename to make it clearer and disambiguate from
places that use Filename as a local var, and initialize it as expected
for ThinLTO. Found this issue while testing changes to dependent patch
D16173.

[pcc]

Seems reasonable to me. Mehdi?

[llvm-commits]

Sure!

[tejohnson]

Great, thanks. Do either of you have any other comments or if not can one of you mark this accepted?

[pcc]

LGTM

tools/gold/gold-plugin.cpp

985–988 ↗

(On Diff #49078)

I don't think this should be dependent on a property of the host machine, as there are behavioral differences between parallelism levels (e.g. symbol ordering will be different, and some uses of inline asm won't work with parallelism >1, although some of that is arguably a bug). Can you please update the comment to reflect that?

[tejohnson]

In D15390#367460, @pcc wrote:

LGTM

Thanks!

tools/gold/gold-plugin.cpp

985–988 ↗

(On Diff #49078)

Ok, will do.

llvm/trunk/test/tools/gold/X86/pr19901_thinlto.ll

+; RUN: llc %s -o %t.o -filetype=obj -relocation-model=pic
+; RUN: llvm-as -function-summary %p/Inputs/pr19901-1.ll -o %t2.o
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=thinlto \
+; RUN:     -shared -m elf_x86_64 -o %t.so %t2.o %t.o
+; RUN: llvm-readobj -t %t.so | FileCheck %s
+
+; CHECK:       Symbol {
+; CHECK:         Name: f
+; CHECK-NEXT:    Value:
+; CHECK-NEXT:    Size:
+; CHECK-NEXT:    Binding: Local
+; CHECK-NEXT:    Type: Function
+; CHECK-NEXT:    Other: {{2|0}}
+; CHECK-NEXT:    Section: .text
+; CHECK-NEXT:  }
+
+target triple = "x86_64-unknown-linux-gnu"
+define i32 @g() {
+  call void @f()
+  ret i32 0
+}
+define linkonce_odr hidden void @f() {
+  ret void
+}

llvm/trunk/test/tools/gold/X86/thinlto.ll

 ; First ensure that the ThinLTO handling in the gold plugin handles
 ; bitcode without function summary sections gracefully.
 ; RUN: llvm-as %s -o %t.o
 ; RUN: llvm-as %p/Inputs/thinlto.ll -o %t2.o
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=thinlto-index-only \
 ; RUN:    -shared %t.o %t2.o -o %t3
+; RUN: not test -e %t3
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    -shared %t.o %t2.o -o %t4
+; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM
 
+; Next generate function summary sections and test gold handling.
 ; RUN: llvm-as -function-summary %s -o %t.o
 ; RUN: llvm-as -function-summary %p/Inputs/thinlto.ll -o %t2.o
 
+; Ensure gold generates an index and not a binary if requested.
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=thinlto-index-only \
 ; RUN:    -shared %t.o %t2.o -o %t3
 ; RUN: llvm-bcanalyzer -dump %t3.thinlto.bc | FileCheck %s --check-prefix=COMBINED
 ; RUN: not test -e %t3
 
+; Ensure gold generates an index as well as a binary by default in ThinLTO mode.
+; First force single-threaded mode
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=jobs=1 \
+; RUN:    -shared %t.o %t2.o -o %t4
+; RUN: llvm-bcanalyzer -dump %t4.thinlto.bc | FileCheck %s --check-prefix=COMBINED
+; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM
+
+; Next force multi-threaded mode
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=jobs=2 \
+; RUN:    -shared %t.o %t2.o -o %t4
+; RUN: llvm-bcanalyzer -dump %t4.thinlto.bc | FileCheck %s --check-prefix=COMBINED
+; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM
+
+; Test --plugin-opt=obj-path to ensure unique object files generated.
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=jobs=2 \
+; RUN:    --plugin-opt=obj-path=%t5.o \
+; RUN:    -shared %t.o %t2.o -o %t4
+; RUN: llvm-nm %t5.o0 | FileCheck %s --check-prefix=NM2
+; RUN: llvm-nm %t5.o1 | FileCheck %s --check-prefix=NM2
+
+; NM: T f
+; NM2: T {{f|g}}
+
 ; COMBINED: <MODULE_STRTAB_BLOCK
 ; COMBINED-NEXT: <ENTRY {{.*}} record string = '{{.*}}/test/tools/gold/X86/Output/thinlto.ll.tmp{{.*}}.o'
 ; COMBINED-NEXT: <ENTRY {{.*}} record string = '{{.*}}/test/tools/gold/X86/Output/thinlto.ll.tmp{{.*}}.o'
 ; COMBINED-NEXT: </MODULE_STRTAB_BLOCK
 ; COMBINED-NEXT: <FUNCTION_SUMMARY_BLOCK
 ; COMBINED-NEXT: <COMBINED_ENTRY
 ; COMBINED-NEXT: <COMBINED_ENTRY
 ; COMBINED-NEXT: </FUNCTION_SUMMARY_BLOCK
 ; COMBINED-NEXT: <VALUE_SYMTAB
 ; Check that the format is: op0=offset, op1=funcguid, where funcguid is
 ; the lower 64 bits of the function name MD5.
 ; COMBINED-NEXT: <COMBINED_FNENTRY abbrevid={{[0-9]+}} op0={{[0-9]+}} op1={{-3706093650706652785|-5300342847281564238}}
 ; COMBINED-NEXT: <COMBINED_FNENTRY abbrevid={{[0-9]+}} op0={{[0-9]+}} op1={{-3706093650706652785|-5300342847281564238}}
 ; COMBINED-NEXT: </VALUE_SYMTAB
 
+declare void @g(...)
+
 define void @f() {
 entry:
+  call void (...) @g()
   ret void
 }

llvm/trunk/tools/gold/gold-plugin.cpp

 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Object/FunctionIndexObjectFile.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/thread.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/Utils/FunctionImportUtils.h"
 #include "llvm/Transforms/Utils/GlobalStatus.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include "llvm/Transforms/Utils/ValueMapper.h"
 #include <list>
 #include <plugin-api.h>
 #include <system_error>
 #include <vector>
 

 struct claimed_file {
   void *handle;
   std::vector<ld_plugin_symbol> syms;
 };
 
 /// RAII wrapper to manage opening and releasing of a ld_plugin_input_file.
 struct PluginInputFile {
   void *Handle;
-  ld_plugin_input_file File;
+  std::unique_ptr<ld_plugin_input_file> File;
 
   PluginInputFile(void *Handle) : Handle(Handle) {
-    if (get_input_file(Handle, &File) != LDPS_OK)
+    File = llvm::make_unique<ld_plugin_input_file>();
+    if (get_input_file(Handle, File.get()) != LDPS_OK)
       message(LDPL_FATAL, "Failed to get file information");
   }
   ~PluginInputFile() {
+    // File would have been reset to nullptr if we moved this object
+    // to a new owner.
+    if (File)
       if (release_input_file(Handle) != LDPS_OK)
         message(LDPL_FATAL, "Failed to release file information");
   }
-  ld_plugin_input_file &file() { return File; }
+
+  ld_plugin_input_file &file() { return *File; }
+
+  PluginInputFile(PluginInputFile &&RHS) = default;
+  PluginInputFile &operator=(PluginInputFile &&RHS) = default;
 };
 
 struct ResolutionInfo {
   bool IsLinkonceOdr = true;
   bool UnnamedAddr = true;
   GlobalValue::VisibilityTypes Visibility = GlobalValue::DefaultVisibility;
   bool CommonInternal = false;
   bool UseCommon = false;
   unsigned CommonSize = 0;
   unsigned CommonAlign = 0;
   claimed_file *CommonFile = nullptr;
 };
+
+/// Class to own information used by a task or during its cleanup for a
+/// ThinLTO backend instantiation.
+class ThinLTOTaskInfo {
+  /// The input file holding the module bitcode read by the ThinLTO task.
+  PluginInputFile InputFile;
+
+  /// The output stream the task will codegen into.
+  std::unique_ptr<raw_fd_ostream> OS;
+
+  /// The file name corresponding to the output stream, used during cleanup.
+  std::string Filename;
+
+  /// Flag indicating whether the output file is a temp file that must be
+  /// added to the cleanup list during cleanup.
+  bool TempOutFile;
+
+public:
+  ThinLTOTaskInfo(PluginInputFile InputFile, std::unique_ptr<raw_fd_ostream> OS,
+                  std::string Filename, bool TempOutFile)
+      : InputFile(std::move(InputFile)), OS(std::move(OS)), Filename(Filename),
+        TempOutFile(TempOutFile) {}
+
+  /// Performs task related cleanup activities that must be done
+  /// single-threaded (i.e. call backs to gold).
+  void cleanup();
+};
 }
 
 static ld_plugin_add_symbols add_symbols = nullptr;
 static ld_plugin_get_symbols get_symbols = nullptr;
 static ld_plugin_add_input_file add_input_file = nullptr;
 static ld_plugin_set_extra_library_path set_extra_library_path = nullptr;
 static ld_plugin_get_view get_view = nullptr;
 static Reloc::Model RelocationModel = Reloc::Default;
enum OutputType {
     OT_BC_ONLY,
     OT_SAVE_TEMPS
   };
   static bool generate_api_file = false;
   static OutputType TheOutputType = OT_NORMAL;
   static unsigned OptLevel = 2;
   // Default parallelism of 0 used to indicate that user did not specify.
   // Actual parallelism default value depends on implementation.
-  // Currently, code generation defaults to no parallelism.
+  // Currently, code generation defaults to no parallelism, whereas
+  // ThinLTO uses the hardware_concurrency as the default.
   static unsigned Parallelism = 0;
 #ifdef NDEBUG
   static bool DisableVerify = true;
 #else
   static bool DisableVerify = false;
 #endif
   static std::string obj_path;
   static std::string extra_library_path;
   static std::string triple;
   static std::string mcpu;
   // When the thinlto plugin option is specified, only read the function
   // the information from intermediate files and write a combined
   // global index for the ThinLTO backends.
   static bool thinlto = false;
+  // If false, all ThinLTO backend compilations through code gen are performed
+  // using multiple threads in the gold-plugin, before handing control back to
+  // gold. If true, exit after creating the combined index, the assuming is
+  // that the build system will launch the backend processes.
+  static bool thinlto_index_only = false;
   // Additional options to pass into the code generator.
   // Note: This array will contain all plugin options which are not claimed
   // as plugin exclusive to pass to the code generator.
   // For example, "generate-api-file" and "as"options are for the plugin
   // use only and will not be passed.
   static std::vector<const char *> extra;
 
   static void process_plugin_option(const char *opt_)
static void process_plugin_option(const char *opt_)
     } else if (opt == "emit-llvm") {
       TheOutputType = OT_BC_ONLY;
     } else if (opt == "save-temps") {
       TheOutputType = OT_SAVE_TEMPS;
     } else if (opt == "disable-output") {
       TheOutputType = OT_DISABLE;
     } else if (opt == "thinlto") {
       thinlto = true;
+    } else if (opt == "thinlto-index-only") {
+      thinlto_index_only = true;
     } else if (opt.size() == 2 && opt[0] == 'O') {
       if (opt[1] < '0' || opt[1] > '3')
         message(LDPL_FATAL, "Optimization level must be between 0 and 3");
       OptLevel = opt[1] - '0';
     } else if (opt.startswith("jobs=")) {
       if (StringRef(opt_ + 5).getAsInteger(10, Parallelism))
         message(LDPL_FATAL, "Invalid parallelism level: %s", opt_ + 5);
     } else if (opt == "disable-verify") {
static ld_plugin_status claim_file_hook(const ld_plugin_input_file *file,
 
   Modules.resize(Modules.size() + 1);
   claimed_file &cf = Modules.back();
 
   cf.handle = file->handle;
 
   // If we are doing ThinLTO compilation, don't need to process the symbols.
   // Later we simply build a combined index file after all files are claimed.
-  if (options::thinlto)
+  if (options::thinlto && options::thinlto_index_only)
     return LDPS_OK;
 
   for (auto &Sym : Obj->symbols()) {
     uint32_t Symflags = Sym.getFlags();
     if (shouldSkip(Symflags))
       continue;
 
     cf.syms.push_back(ld_plugin_symbol());
static void recordFile(std::string Filename, bool TempOutFile) {
   if (add_input_file(Filename.c_str()) != LDPS_OK)
     message(LDPL_FATAL,
             "Unable to add .o file to the link. File left behind in: %s",
             Filename.c_str());
   if (TempOutFile)
     Cleanup.push_back(Filename.c_str());
 }
 
+void ThinLTOTaskInfo::cleanup() {
+  // Close the output file descriptor before we pass it to gold.
+  OS->close();
+
+  recordFile(Filename, TempOutFile);
+}
+
 namespace {
-/// Class to manage optimization and code generation for a module.
+/// Class to manage optimization and code generation for a module, possibly
+/// in a thread (ThinLTO).
 class CodeGen {
   /// The module for which this will generate code.
   std::unique_ptr<llvm::Module> M;
 
+  /// The output stream to generate code into.
+  raw_fd_ostream *OS;
+
+  /// The task ID when this was invoked in a thread (ThinLTO).
+  int TaskID;
+
+  /// The function index for ThinLTO tasks.
+  const FunctionInfoIndex *CombinedIndex;
+
   /// The target machine for generating code for this module.
   std::unique_ptr<TargetMachine> TM;
 
+  /// Filename to use as base when save-temps is enabled, used to get
+  /// a unique and identifiable save-temps output file for each ThinLTO backend.
+  std::string SaveTempsFilename;
+
 public:
   /// Constructor used by full LTO.
-  CodeGen(std::unique_ptr<llvm::Module> M) : M(std::move(M)) {
+  CodeGen(std::unique_ptr<llvm::Module> M)
+      : M(std::move(M)), OS(nullptr), TaskID(-1), CombinedIndex(nullptr) {
+    initTargetMachine();
+  }
+  /// Constructor used by ThinLTO.
+  CodeGen(std::unique_ptr<llvm::Module> M, raw_fd_ostream *OS, int TaskID,
+          const FunctionInfoIndex *CombinedIndex, std::string Filename)
+      : M(std::move(M)), OS(OS), TaskID(TaskID), CombinedIndex(CombinedIndex),
+        SaveTempsFilename(Filename) {
+    assert(options::thinlto == !!CombinedIndex &&
+           "Expected function index iff performing ThinLTO");
     initTargetMachine();
   }
 
   /// Invoke LTO passes and the code generator for the module.
   void runAll();
 
+  /// Invoke the actual code generation to emit Module's object to file.
+  void runCodegenPasses();
+
 private:
   /// Create a target machine for the module. Must be unique for each
   /// module/task.
   void initTargetMachine();
 
   /// Run all LTO passes on the module.
   void runLTOPasses();
 
void CodeGen::runLTOPasses() {
   PMB.Inliner = createFunctionInliningPass();
   // Unconditionally verify input since it is not verified before this
   // point and has unknown origin.
   PMB.VerifyInput = true;
   PMB.VerifyOutput = !options::DisableVerify;
   PMB.LoopVectorize = true;
   PMB.SLPVectorize = true;
   PMB.OptLevel = options::OptLevel;
+  PMB.FunctionIndex = CombinedIndex;
   PMB.populateLTOPassManager(passes);
   passes.run(*M);
 }
 
 /// Open a file and return the new file descriptor given a base input
 /// file name, a flag indicating whether a temp file should be generated,
 /// and an optional task id. The new filename generated is
 /// returned in \p NewFilename.
if (TempOutFile) {
     std::error_code EC =
         sys::fs::openFileForWrite(NewFilename, FD, sys::fs::F_None);
     if (EC)
       message(LDPL_FATAL, "Could not open file: %s", EC.message().c_str());
   }
   return FD;
 }
 
+void CodeGen::runCodegenPasses() {
+  assert(OS && "Output stream must be set before emitting to file");
+  legacy::PassManager CodeGenPasses;
+  if (TM->addPassesToEmitFile(CodeGenPasses, *OS,
+                              TargetMachine::CGFT_ObjectFile))
+    report_fatal_error("Failed to setup codegen");
+  CodeGenPasses.run(*M);
+}
+
 void CodeGen::runSplitCodeGen() {
   const std::string &TripleStr = M->getTargetTriple();
   Triple TheTriple(TripleStr);
 
   SubtargetFeatures Features = getFeatures(TheTriple);
 
   TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
   CodeGenOpt::Level CGOptLevel = getCGOptLevel();
void CodeGen::runSplitCodeGen() {
   for (auto &Filename : Filenames)
     recordFile(Filename.c_str(), TempOutFile);
 }
 
 void CodeGen::runAll() {
   runLTOPasses();
 
   if (options::TheOutputType == options::OT_SAVE_TEMPS) {
-    saveBCFile(output_name + ".opt.bc", *M);
+    std::string OptFilename = output_name;
+    // If the CodeGen client provided a filename, use it. Always expect
+    // a provided filename if we are in a task (i.e. ThinLTO backend).
+    assert(!SaveTempsFilename.empty() || TaskID == -1);
+    if (!SaveTempsFilename.empty())
+      OptFilename = SaveTempsFilename;
+    saveBCFile(OptFilename + ".opt.bc", *M);
   }
 
+  // If we are already in a thread (i.e. ThinLTO), just perform
+  // codegen passes directly.
+  if (TaskID >= 0)
+    runCodegenPasses();
+  // Otherwise attempt split code gen.
+  else
     runSplitCodeGen();
 }
 
 /// Links the module in \p View from file \p F into the combined module
 /// saved in the IRMover \p L. Returns true on error, false on success.
 static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
                          const void *View, ld_plugin_input_file &File,
                          raw_fd_ostream *ApiFile, StringSet<> &Internalize,
                          StringSet<> &Maybe) {
   std::vector<GlobalValue *> Keep;
   std::unique_ptr<Module> M = getModuleForFile(Context, F, View, File, ApiFile,
                                                Internalize, Maybe, Keep);
   if (!M.get())
     return false;
   if (!options::triple.empty())
     M->setTargetTriple(options::triple.c_str());
   else if (M->getTargetTriple().empty()) {
     M->setTargetTriple(DefaultTriple);
   }
 
   if (L.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {}))
     return true;
   return false;
 }
 
+/// Perform the ThinLTO backend on a single module, invoking the LTO and codegen
+/// pipelines.
+static void thinLTOBackendTask(claimed_file &F, const void *View,
+                               ld_plugin_input_file &File,
+                               raw_fd_ostream *ApiFile,
+                               const FunctionInfoIndex &CombinedIndex,
+                               raw_fd_ostream *OS, unsigned TaskID) {
+  // Need to use a separate context for each task
+  LLVMContext Context;
+  Context.setDiagnosticHandler(diagnosticHandlerForContext, nullptr, true);
+
+  std::unique_ptr<llvm::Module> NewModule(new llvm::Module(File.name, Context));
+  IRMover L(*NewModule.get());
+
+  StringSet<> Dummy;
+  if (linkInModule(Context, L, F, View, File, ApiFile, Dummy, Dummy))
+    message(LDPL_FATAL, "Failed to rename module for ThinLTO");
+  if (renameModuleForThinLTO(*NewModule, &CombinedIndex))
+    message(LDPL_FATAL, "Failed to rename module for ThinLTO");
+
+  CodeGen codeGen(std::move(NewModule), OS, TaskID, &CombinedIndex, File.name);
+  codeGen.runAll();
+}
+
+/// Launch each module's backend pipeline in a separate task in a thread pool.
+static void thinLTOBackends(raw_fd_ostream *ApiFile,
+                            const FunctionInfoIndex &CombinedIndex) {
+  unsigned TaskCount = 0;
+  std::vector<ThinLTOTaskInfo> Tasks;
+  Tasks.reserve(Modules.size());
+  unsigned int MaxThreads = options::Parallelism
+                                ? options::Parallelism
+                                : thread::hardware_concurrency();
+
+  // Create ThreadPool in nested scope so that threads will be joined
+  // on destruction.
+  {
+    ThreadPool ThinLTOThreadPool(MaxThreads);
+    for (claimed_file &F : Modules) {
+      // Do all the gold callbacks in the main thread, since gold is not thread
+      // safe by default.
+      PluginInputFile InputFile(F.handle);
+      const void *View = getSymbolsAndView(F);
+
+      SmallString<128> Filename;
+      if (!options::obj_path.empty())
+        // Note that openOutputFile will append a unique ID for each task
+        Filename = options::obj_path;
+      else if (options::TheOutputType == options::OT_SAVE_TEMPS) {
+        // Use the input file name so that we get a unique and identifiable
+        // output file for each ThinLTO backend task.
+        Filename = InputFile.file().name;
+        Filename += ".thinlto.o";
+      }
+      bool TempOutFile = Filename.empty();
+
+      SmallString<128> NewFilename;
+      int FD = openOutputFile(Filename, TempOutFile, NewFilename,
+                              // Only append the TaskID if we will use the
+                              // non-unique obj_path.
+                              !options::obj_path.empty() ? TaskCount : -1);
+      TaskCount++;
+      std::unique_ptr<raw_fd_ostream> OS =
+          llvm::make_unique<raw_fd_ostream>(FD, true);
+
+      // Enqueue the task
+      ThinLTOThreadPool.async(thinLTOBackendTask, std::ref(F), View,
+                              std::ref(InputFile.file()), ApiFile,
+                              std::ref(CombinedIndex), OS.get(), TaskCount);
+
+      // Record the information needed by the task or during its cleanup
+      // to a ThinLTOTaskInfo instance. For information needed by the task
+      // the unique_ptr ownership is transferred to the ThinLTOTaskInfo.
+      Tasks.emplace_back(std::move(InputFile), std::move(OS),
+                         NewFilename.c_str(), TempOutFile);
+    }
+  }
+
+  for (auto &Task : Tasks)
+    Task.cleanup();
+}
+
 /// gold informs us that all symbols have been read. At this point, we use
 /// get_symbols to see if any of our definitions have been overridden by a
 /// native object file. Then, perform optimization and codegen.
 static ld_plugin_status allSymbolsReadHook(raw_fd_ostream *ApiFile) {
   if (Modules.empty())
     return LDPS_OK;
 
   if (unsigned NumOpts = options::extra.size())
if (options::thinlto) {
     raw_fd_ostream OS(output_name + ".thinlto.bc", EC,
                       sys::fs::OpenFlags::F_None);
     if (EC)
       message(LDPL_FATAL, "Unable to open %s.thinlto.bc for writing: %s",
               output_name.data(), EC.message().c_str());
     WriteFunctionSummaryToFile(CombinedIndex, OS);
     OS.close();
 
+    if (options::thinlto_index_only) {
       cleanup_hook();
       exit(0);
     }
 
+    thinLTOBackends(ApiFile, CombinedIndex);
+    return LDPS_OK;
+  }
+
   LLVMContext Context;
   Context.setDiagnosticHandler(diagnosticHandlerForContext, nullptr, true);
 
   std::unique_ptr<Module> Combined(new Module("ld-temp.o", Context));
   IRMover L(*Combined);
 
   StringSet<> Internalize;
   StringSet<> Maybe;