[PATCH] D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I have mostly small suggestions below. Overall, it looks pretty good and straightforward.



================
Comment at: llvm/lib/LTO/LTO.cpp:423
 
+BitcodeModule &InputFile::getBitcodeModuleForCG() {
+  assert(Mods.size() == 1 && "Legacy CodeGenerator only supports one module");
----------------
Suggest renaming to something more generic like "getSingleBitcodeModule", and just change the assert text to "Expect only one module". In case we want to use this interface for something other than that client in the future.


================
Comment at: llvm/tools/llvm-lto/llvm-lto.cpp:458
 
-  if (ThinLTOModuleId.getNumOccurrences()) {
-    if (InputFilenames.size() != 1)
-      report_fatal_error("Can't override the module id for multiple files");
-    M->setModuleIdentifier(ThinLTOModuleId);
+static std::unique_ptr<lto::InputFile> loadInputFile(MemoryBufferRef Buffer) {
+  ExitOnError ExitOnErr("llvm-lto: error loading input '" +
----------------
Maybe combine this with loadFile, since they are always used together and the Buffer from loadFile doesn't seem to be used elsewhere?


================
Comment at: llvm/tools/llvm-lto/llvm-lto.cpp:462
 
-  if (ThinLTOModuleId.getNumOccurrences()) {
-    if (InputFilenames.size() != 1)
----------------
The new patch loses this option, which seems to be used by a couple tests under test/ThinLTO/X86.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60226/new/

https://reviews.llvm.org/D60226