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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

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


================
Comment at: tools/gold/gold-plugin.cpp:80
@@ +79,3 @@
+
+/// Class to own information used by a thread or during its join.
+class ThreadInfo {
----------------
rafael wrote:
> thread or task now?
Yeah, will change to TaskInfo and update the comments accordingly.

================
Comment at: tools/gold/gold-plugin.cpp:920
@@ +919,3 @@
+/// pipelines.
+static void ThinLTOBackendThread(claimed_file &F, const void *View,
+                                 ld_plugin_input_file &File,
----------------
rafael wrote:
> Start the function name with a lower case.
> 
> This is not a thread anymore. Task maybe?
> 
Will fix both issues

================
Comment at: tools/gold/gold-plugin.cpp:925
@@ +924,3 @@
+                                 raw_fd_ostream *OS) {
+  // Not thread safe, open a new one for each thread.
+  LLVMContext Context;
----------------
rafael wrote:
> It is thread safe since you create a new one for each thread, no?
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.

================
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?

================
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?


http://reviews.llvm.org/D15390