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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: tools/gold/gold-plugin.cpp:805
@@ +804,3 @@
+/// in a thread (ThinLTO or split code generation). In the split code generation
+/// case, a new CodeGen object will be created for each split module part.
+class CodeGen {
----------------
joker.eph wrote:
> 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.
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.

================
Comment at: tools/gold/gold-plugin.cpp:1043
@@ +1042,3 @@
+
+  CodegenThreadPool.wait();
+
----------------
joker.eph wrote:
> Usually I prefer RAII (i.e. using a new scope).
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.

================
Comment at: tools/gold/gold-plugin.cpp:1121
@@ +1120,3 @@
+  unsigned TaskCount = 0;
+  std::vector<std::unique_ptr<ThinLTOTaskInfo>> Tasks;
+  unsigned int MaxThreads = options::Parallelism
----------------
joker.eph wrote:
> This could be 
> 
> ```
> std::vector<ThinLTOTaskInfo> Tasks;
> Tasks.reserve(Modules.size());
> ```
> 
> (same above for ` std::vector<std::unique_ptr<TaskInfo>> Tasks;` around line 1023)
Ok

================
Comment at: tools/gold/gold-plugin.cpp:1169
@@ -851,1 +1168,3 @@
+  for (auto &Task : Tasks)
+    Task->cleanup();
 }
----------------
joker.eph wrote:
> Could this be done in the TaskInfo dtor?
I previously had it there, but Rafael thought the dtor was too heavy-weight and wanted it more explicit. =)


http://reviews.llvm.org/D15390