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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Some more comment.


================
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 {
----------------
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.

================
Comment at: tools/gold/gold-plugin.cpp:820
@@ +819,3 @@
+  /// The target machine for generating code for this module.
+  std::unique_ptr<TargetMachine> TM;
+
----------------
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)

================
Comment at: tools/gold/gold-plugin.cpp:1043
@@ +1042,3 @@
+
+  CodegenThreadPool.wait();
+
----------------
Usually I prefer RAII (i.e. using a new scope).

================
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
----------------
This could be 

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

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

================
Comment at: tools/gold/gold-plugin.cpp:1169
@@ -851,1 +1168,3 @@
+  for (auto &Task : Tasks)
+    Task->cleanup();
 }
----------------
Could this be done in the TaskInfo dtor?


http://reviews.llvm.org/D15390