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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

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.


================
Comment at: tools/gold/gold-plugin.cpp:891
@@ +890,3 @@
+  // Run backend threads.
+  splitCodeGen(std::move(M), OSPtrs.vec(), options::mcpu, Features.getString(),
+               Options, RelocationModel, CodeModel::Default, CGOptLevel);
----------------
rafael wrote:
> splitCodeGen can take a ArrayRef too. Why do you need the vec?
> 
Ah ok, fixed.

================
Comment at: tools/gold/gold-plugin.cpp:975
@@ +974,3 @@
+
+TaskInfo::~TaskInfo() {
+  // Close the output file descriptor before we pass it to gold.
----------------
rafael wrote:
> 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();
> 
Changed TaskInfo::~TaskInfo into TaskInfo::cleanup and invoked explicitly on each task after the wait().

================
Comment at: tools/gold/gold-plugin.cpp:998
@@ +997,3 @@
+
+  std::vector<claimed_file *> Worklist;
+  for (claimed_file &F : Modules)
----------------
rafael wrote:
> Why do you need a worklist?
> 
> Can't you just just a simple loop over Modules?
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.

================
Comment at: tools/gold/gold-plugin.cpp:1033
@@ +1032,3 @@
+        std::move(InputFile), std::move(OS), NewFilename.c_str(), TempOutFile);
+    Tasks.emplace_back(std::move(Task));
+  }
----------------
rafael wrote:
> This can be just
> 
>     Tasks.emplace_back(new TaskInfo(std::move(InputFile), std::move(OS),
>                                     NewFilename.c_str(), TempOutFile));
Fixed


http://reviews.llvm.org/D15390