[PATCH] D18999: [ELF/LTO] Parallel Codegen for LLD

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:311
@@ -310,2 +310,3 @@
     error("invalid optimization level for LTO: " + getString(Args, OPT_lto_O));
+  Config->CGThreads = getInteger(Args, OPT_jobs, 1);
 
----------------
Move this before Config->Optimize to sort these lines.

================
Comment at: ELF/LTO.cpp:38
@@ -37,1 +37,3 @@
+static void saveLtoObjectFile(StringRef Buffer, unsigned I, bool Many) {
   std::error_code EC;
+  SmallString<128> FileName = Config->OutputFile;
----------------
Move this line just before `raw_fd_ostream OS...` so that the definition gets closer to use.

================
Comment at: ELF/LTO.cpp:39
@@ -37,3 +38,3 @@
   std::error_code EC;
-  raw_fd_ostream OS(Config->OutputFile.str() + ".lto.o", EC,
-                    sys::fs::OpenFlags::F_None);
+  SmallString<128> FileName = Config->OutputFile;
+  FileName += (Many) ? utostr(I) : "";
----------------
I'd spell this variable `Filename` as one word.

================
Comment at: ELF/LTO.cpp:40
@@ +39,3 @@
+  SmallString<128> FileName = Config->OutputFile;
+  FileName += (Many) ? utostr(I) : "";
+  FileName += ".lto.o";
----------------
rafael wrote:
> Drop the () around many.
I found that appending an empty string is a bit confusing here. I'd probably do

  if (Many)
    Filename += utostr(I);

================
Comment at: ELF/LTO.cpp:142
@@ -138,1 +141,3 @@
 
+static SubtargetFeatures getFeatures(Triple &TheTriple) {
+  SubtargetFeatures Features;
----------------
davide wrote:
> davide wrote:
> > rafael wrote:
> > > Is this related to parallel LTO?
> > you use the returned type as argument to codeine.
> *codegen.
This function needs a comment.

================
Comment at: ELF/LTO.cpp:145
@@ +144,3 @@
+  Features.getDefaultSubtargetFeatures(TheTriple);
+  for (const std::string &A : MAttrs)
+    Features.AddFeature(A);
----------------
Where does this `MAttrs` come from?

================
Comment at: ELF/LTO.cpp:165
@@ +164,3 @@
+std::vector<std::unique_ptr<InputFile>> BitcodeCompiler::runSplitCodegen() {
+  unsigned NumThreads = (Config->CGThreads >= 1) ? Config->CGThreads : 1;
+  OwningData.resize(NumThreads);
----------------
Why don't you just use Config->CGThreads as the number of threads?

================
Comment at: ELF/LTO.cpp:184
@@ +183,3 @@
+  std::vector<std::unique_ptr<InputFile>> ObjFiles;
+  for (SmallString<0> &Obj : OwningData) {
+    ObjFiles.push_back(createObjectFile(
----------------
Remove {}.

================
Comment at: ELF/Options.td:237-238
@@ -237,1 +236,4 @@
+// LTO-related options.
+def jobs : Joined<["--"], "jobs=">,
+  HelpText<"Number of threads to run codegen">;
 def disable_verify : Flag<["-"], "disable-verify">;
----------------
I prefer --thread-count because gold has that option.

================
Comment at: ELF/SymbolTable.cpp:117
@@ +116,3 @@
+
+    // Replace bitcode symbols.
+    llvm::DenseSet<StringRef> DummyGroups;
----------------
Move this code before the enclosing for-loop. (Because the entire for-loop is to replace bitcode symbols.)


http://reviews.llvm.org/D18999