[PATCH] D12260: CodeGen: Introduce LinkedCodeGen and teach LTOCodeGenerator to use it.

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

A few comments.


================
Comment at: lib/CodeGen/Parallel.cpp:70
@@ +69,3 @@
+    WriteBitcodeToFile(MPart.get(), OS);
+    OS.flush();
+
----------------
Add a comment? Something like: `we need to do this because we want to clone the module in a new context to multi-thread the codegen. The "dumping()" part has to be performed in the caller thread and not the one we create just after because we are still in the old LLVMContext` 

It seems LLVM is missing the ability to clone a module in a new context directly...

================
Comment at: lib/CodeGen/Parallel.cpp:72
@@ +71,3 @@
+
+    Threads.emplace_back([=](llvm::raw_pwrite_stream *OS, StringRef BC) {
+      LLVMContext Ctx;
----------------
What is copied? Might replace the `=` with an explicit list if possible. I'm not sure why you want to copy "Options" for instance? Also, some values are passed as argument to the function, the cut is not clear to me.

================
Comment at: lib/LTO/LTOCodeGenerator.cpp:504
@@ -502,3 +503,3 @@
 
   legacy::PassManager codeGenPasses;
 
----------------
rename, not longer involved in CodeGen

================
Comment at: tools/llvm-lto/llvm-lto.cpp:253
@@ -250,7 +252,3 @@
 
-    std::error_code EC;
-    raw_fd_ostream FileStream(OutputFilename, EC, sys::fs::F_None);
-    if (EC) {
-      errs() << argv[0] << ": error opening the file '" << OutputFilename
-             << "': " << EC.message() << "\n";
-      return 1;
+    std::list<raw_fd_ostream> OSs;
+    std::vector<raw_pwrite_stream *> OSPtrs;
----------------
Tools are usually using `tool_output_file`, is it appropriate here?

================
Comment at: tools/llvm-lto/llvm-lto.cpp:275
@@ +274,3 @@
+    if (Parallelism != 1) {
+      errs() << argv[0] << ": -j must be specified together with -o\n";
+      return 1;
----------------
Why? I mean any particular reason or just for convenience of the implementation?


http://reviews.llvm.org/D12260