[PATCH] Initial implementation of link-time optimization for the new COFF linker.

[Rui Ueyama]

This is very small amount code, and all the things seem to fit naturally to the existing things. Very nice.


================
Comment at: COFF/InputFiles.cpp:255
@@ +254,3 @@
+                                              MBRef.getBufferSize(),
+                                              llvm::TargetOptions(), Err));
+  if (!Err.empty())
----------------
Use parentheses for ifs having elses for consistency.

================
Comment at: COFF/InputFiles.cpp:265
@@ +264,3 @@
+    else
+      SymbolBodies.push_back(new (Alloc) DefinedBitcode(SymName));
+  }
----------------
Ditto

================
Comment at: COFF/InputFiles.h:166
@@ -164,1 +165,3 @@
 
+class BitcodeFile : public InputFile {
+public:
----------------
Can you add a brief comment for this class saying that this is for LTO, just like I did for other classes?

================
Comment at: COFF/InputFiles.h:172
@@ +171,3 @@
+  StringRef getName() override { return Name; }
+  virtual std::vector<SymbolBody *> &getSymbols() { return SymbolBodies; }
+
----------------
Use override instead of virtual.

================
Comment at: COFF/InputFiles.h:175
@@ +174,3 @@
+  llvm::LTOModule *getModule() const { return M.get(); }
+  llvm::LTOModule *releaseModule() { return M.release(); }
+
----------------
Add using llvm::LTOModule at beginning of this file.

================
Comment at: COFF/InputFiles.h:180
@@ +179,3 @@
+
+  std::string Name;
+  MemoryBufferRef MBRef;
----------------
Name -> Filename.

================
Comment at: COFF/SymbolTable.cpp:65
@@ -61,1 +64,3 @@
 
+std::error_code SymbolTable::addBitcode(BitcodeFile *File) {
+  BitcodeFiles.emplace_back(File);
----------------
Move this piece of code after addArchive(ArchiveFile *) for consistent order.

================
Comment at: COFF/SymbolTable.cpp:193
@@ +192,3 @@
+  llvm::LTOCodeGenerator CG;
+
+  std::set<DefinedBitcode *> PreservedBitcodeSymbols;
----------------
Remove this blank line.

================
Comment at: COFF/SymbolTable.cpp:197
@@ +196,3 @@
+  // All symbols referenced by non-bitcode objects must be preserved.
+  for (auto &Obj : ObjectFiles)
+    for (SymbolBody *Body : Obj->getSymbols())
----------------
Use real type instead of auto.

================
Comment at: COFF/SymbolTable.cpp:210
@@ +209,3 @@
+  CG.setModule(BitcodeFiles[0]->releaseModule());
+  for (unsigned I = 1; I != BitcodeFiles.size(); ++I)
+    CG.addModule(BitcodeFiles[I]->getModule());
----------------
You want to cache size() -- for (unsigned I = 1, E = BitcodeFiles.size(), I != E ...)

================
Comment at: COFF/SymbolTable.cpp:218-236
@@ +217,21 @@
+
+  // Undefine the preserved bitcode symbols. They are about to be replaced with
+  // real symbols. Any other bitcode symbols are removed from the symbol table,
+  // as they are no longer needed, and will confuse the writer.
+  for (auto I = Symtab.begin(), E = Symtab.end(); I != E;) {
+    StringRef Name = I->first;
+    Symbol *Ref = I->second;
+    if (auto *S = dyn_cast<DefinedBitcode>(Ref->Body)) {
+      if (PreservedBitcodeSymbols.count(S)) {
+        Ref->Body = new (Alloc) Undefined(Name);
+        ++I;
+      } else {
+        auto OldI = I;
+        ++I;
+        Symtab.erase(OldI);
+      }
+    } else {
+      ++I;
+    }
+  }
+
----------------
What does this code do? Why do we need to insert new Undefined symbols at this point?

Ah, I think I understand what you are doing. LTO object file is added after all files are resolved, and at this moment we are about to add "duplicate" symbols, correct?

Maybe a better approach is to tweak Defined::compare so that any symbol whose type is of DefinedBitcode always "win" over any other symbols.

================
Comment at: COFF/SymbolTable.cpp:238
@@ +237,3 @@
+
+  addFile(llvm::make_unique<ObjectFile>("<LTO object>",
+                                        LTOObjectFile->getMemBufferRef()));
----------------
You want to return addFile's return value.

================
Comment at: COFF/SymbolTable.h:53
@@ +52,3 @@
+  // Build a COFF object representing the combined contents of BitcodeFiles
+  // and add it to the symbol table.
+  std::error_code addCombinedLTOObject();
----------------
Also mention that that this function is called after all files are added and before writer writes results to a file.

================
Comment at: COFF/Symbols.cpp:85
@@ -84,1 +84,3 @@
+  if (Magic == file_magic::bitcode)
+    return std::unique_ptr<InputFile>(llvm::make_unique<BitcodeFile>(MBRef));
   if (Magic == file_magic::coff_import_library)
----------------
Use new instead of make_unique with consistency with other lines in this function.

http://reviews.llvm.org/D10115

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/