[PATCH] D23778: [ThinLTO] Move ThinLTOCodeGenerator implementation to target the new LTO API.

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Sorry for the delay.

I didn't go through the test changes in any detail - what is the main reason the new API caused changes to the way llvm-lto worked and the resulting test behavior?


================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:208
@@ -204,3 +207,3 @@
    */
-  void promote(Module &Module, ModuleSummaryIndex &Index);
+  void promote(std::function<void(unsigned, const Module &)> Callback);
 
----------------
Document callback here and in the other changed interfaces, and remove description of Index being updated since it is no longer passed.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:214
@@ -513,3 +213,3 @@
  * Perform promotion and renaming of exported internal functions.
  * Index is updated to reflect linkage changes from weak resolution.
  */
----------------
Comment needs update here and for other changed interfaces.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:357
@@ +356,3 @@
+  std::vector<std::unique_ptr<MemoryBuffer>> OutputBuffers;
+  OutputBuffers.resize(Codegen.getMaxTasks());
+  // Callback for the LTO API.
----------------
This variable never used.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:363
@@ +362,3 @@
+      std::unique_ptr<raw_pwrite_stream> getStream() override {
+        report_fatal_error("Unexpected");
+      }
----------------
I'm a little confused about what this interface is used for - why are we asserting if there is any stream requested?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:418
@@ +417,3 @@
+    std::unique_ptr<lto::InputFile> Input = std::move(*InputOrErr);
+    // Iterate symbols in the input file. We only gets global symbol here, so
+    // the only logic we need is to mark prevailing the first copy of a weak
----------------
s/only gets global symbol/only get global symbols/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:425
@@ +424,3 @@
+
+      if (!(Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)) {
+        auto &Entry = PrevailingSymbols[Sym.getName()];
----------------
early continue instead?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:434
@@ +433,3 @@
+          Entry.first = Input.get();
+          Entry.second = (Sym.getFlags() & (object::BasicSymbolRef::SF_Weak |
+                                            object::BasicSymbolRef::SF_Common));
----------------
Maybe combine with earlier duplicated flag checks into a variable like IsWeak

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:444
@@ +443,3 @@
+  // the CodeGen.
+  for (auto &Input : Inputs) {
+    std::vector<lto::SymbolResolution> Resolutions;
----------------
I believe this loop nest can be merged with the above (since the first copy always selected as prevailing)


https://reviews.llvm.org/D23778