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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/Parallel.cpp:44
@@ +43,3 @@
+  if (TM->addPassesToEmitFile(CodeGenPasses, OS,
+                              TargetMachine::CGFT_ObjectFile))
+    report_fatal_error("Failed to setup codegen");
----------------
This doesn't seem like it will work, as we do have a client that does not own the stream (`LTOCodeGenerator::compileOptimizedToFile`, and now llvm-lto). Let's keep the existing API for now.

================
Comment at: lib/CodeGen/Parallel.cpp:70
@@ +69,3 @@
+    WriteBitcodeToFile(MPart.get(), OS);
+    OS.flush();
+
----------------
joker.eph wrote:
> 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...
Done

================
Comment at: lib/CodeGen/Parallel.cpp:72
@@ +71,3 @@
+
+    Threads.emplace_back([=](llvm::raw_pwrite_stream *OS, StringRef BC) {
+      LLVMContext Ctx;
----------------
joker.eph wrote:
> 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.
This is now a list of copied things.

It seems that we don't need to copy Options, but this is a micro-optimization not worth thinking about, so I'd rather just copy it.

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

================
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;
----------------
joker.eph wrote:
> Tools are usually using `tool_output_file`, is it appropriate here?
Yes, I suppose so -- we might want to clean up if one of the files cannot be opened. Done.

================
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;
----------------
joker.eph wrote:
> Why? I mean any particular reason or just for convenience of the implementation?
The latter. Since we wouldn't be able to gain any extra test coverage by supporting this case, we don't need to.


http://reviews.llvm.org/D12260