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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D15390#338559, @joker.eph wrote:

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


Thanks for the comments!


================
Comment at: tools/gold/gold-plugin.cpp:102
@@ +101,3 @@
+    File = std::move(RHS.File);
+  }
+  PluginInputFile &operator=(PluginInputFile &&RHS) {
----------------
joker.eph wrote:
> Any difference with ` PluginInputFile(PluginInputFile &&RHS) = default;` ?
Good point, will change to default

================
Comment at: tools/gold/gold-plugin.cpp:107
@@ -88,3 +106,3 @@
+    return *this;
   }
 };
----------------
joker.eph wrote:
> (same =default here)
Ditto.

================
Comment at: tools/gold/gold-plugin.cpp:826
@@ +825,3 @@
+  /// The target machine for generating code for this module.
+  std::unique_ptr<TargetMachine> TM;
+
----------------
joker.eph wrote:
> Note: you could reuse the TargetMachine for the next module processed by this thread.
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?

================
Comment at: tools/gold/gold-plugin.cpp:1104
@@ +1103,3 @@
+  if (L.move(*M, Keep, [](GlobalValue &, IRMover::ValueAdder) {}))
+    message(LDPL_FATAL, "Failed to rename module for ThinLTO");
+  if (renameModuleForThinLTO(*NewModule, &CombinedIndex))
----------------
joker.eph wrote:
> There is a bunch of duplicated code above (used in regular LTO as well I think)
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.


http://reviews.llvm.org/D15390