[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm-c/lto.h:668
@@ +667,3 @@
+ * the disk. Set to 100 to indicate no limit, 50 to indicate that the cache size
+ * will not increase over the free space. A value over 100 will be reduced to
+ * 100.
----------------
Is the max cache size set via the space that was available at the start of the compilation, or is the threshold updated so that if something else comes along and eats up some disk space the allowable max cache size is adjusted downward?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:47
@@ +46,3 @@
+/// Cache behavior controls.
+struct CachingOptions {
+  std::string Path;
----------------
It just wasn't clear how they interact, although your explanation was what I guessed. Maybe for the thinlto_codegen_set_cache_entry_expiration interface add that the module may be removed earlier due to the pruning interval.

================
Comment at: lib/LTO/LTOModule.cpp:80
@@ +79,3 @@
+  // Right now the detection is only based on the summary presence. We may want
+  // to add a dedicated flag at some point.
+  return hasFunctionSummary(IRFile->getMemoryBufferRef(),
----------------
Also for future consider caching value unless it is expected to be called only once per module.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:66
@@ +65,3 @@
+  } else {
+    ModuleOrErr = parseBitcodeFile(Buffer, Context);
+  }
----------------
The when lazy metadata linking is enabled, metadata parsing should be postponed until we actually go to do the import and linkInModule during the bulk importing. Since the source module is destroyed at the end of linkInModule, this should reduce the max amount of live metadata to one module at a time, rather than all modules we are importing from. I'm surprised it didn't reduce the max memory usage in a debug build.

If we ever go back to post-pass metadata linking in the function importer (i.e. if we decide to do iterative importing rather than a single bulk import from each module), this will become more critical. However, if we move to summary-only importing decisions it will obviate the need to do this.

Until we go to summary only import decisions, I would think it should reduce the max memory usage as per my first paragraph. Did you see a cost going with lazy metadata loading?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:85
@@ +84,3 @@
+  std::error_code EC;
+  raw_fd_ostream OS(SaveTempPath.str(), EC, sys::fs::F_None);
+  if (EC)
----------------
Is this a TODO? If so, please add TODO comment.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:109
@@ +108,3 @@
+  LLVMContext &Context;
+  StringMap<MemoryBufferRef> &ModuleMap;
+
----------------
Add doxygyen comment describing ModuleMap.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:113
@@ +112,3 @@
+  ModuleLoader(LLVMContext &Context, StringMap<MemoryBufferRef> &ModuleMap)
+      : Context(Context), ModuleMap(ModuleMap) {}
+
----------------
Ok. When you put this support back in your description above ("don't need to do anything thing you won't internalize any more something already private") is better than the comment about restriction.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:330
@@ +329,3 @@
+
+  // Save temps: index.
+  if (!SaveTempsDir.empty()) {
----------------
Ok, but I think you still want the linkonce/weak linkage changes? When you put this back this is more reason to split the internalization and linkonce/weak linkage changes into two routines and only invoke the latter if the sets are empty.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:358
@@ +357,3 @@
+        if (!SaveTempsDir.empty()) {
+          auto TheModule = loadModuleFromBuffer(ModuleBuffer, Context, false);
+          saveTempBitcode(*TheModule, SaveTempsDir, count, ".0.original.bc");
----------------
What is the difference between this ModuleBuffer and the one loaded out of the ModuleMap in the below call to loadModuleFromBuffer? If they are the same, can loadModuleFromBuffer be called a single time and the resulting module optionally saved after?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:378
@@ +377,2 @@
+    llvm::PrintStatistics();
+}
----------------
Unfortunately this means that ThinLTOCodeGenerator::run() is currently untested. Consider adding a mode to llvm-lto that does all thinlto steps (thin-action=all?) to test all the steps via this interface.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:82
@@ +81,3 @@
+                                         "cross-module importing (requires "
+                                         "-thinlto-index)."),
+        clEnumValN(THINOPT, "optimize", "Perform ThinLTO optimizations."),
----------------
Actual option below is "functionindex". But per the ref graph patch, this is broader than a function index, so I am looking at changing references to function index to something else anyway. So I would suggest changing the actual option below to "thinlto-index".

================
Comment at: tools/llvm-lto/llvm-lto.cpp:361
@@ +360,3 @@
+  /// the index and add them to the generator, finally perform the promotion
+  /// on the files mentionned on the command line (these must match the index
+  /// content).
----------------
s/mentionned/mentioned/ (here and below for import()).

================
Comment at: tools/llvm-lto/llvm-lto.cpp:373
@@ +372,3 @@
+    for (auto &MemBuffer : InputBuffers)
+      ThinGenerator.addModule(MemBuffer->getBufferIdentifier(),
+                              MemBuffer->getBuffer());
----------------
Adding all the modules isn't needed for promotion. Should be able to remove this loop.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:398
@@ +397,3 @@
+  void import() {
+    if (InputFilenames.size() != 1 && !OutputFilename.empty())
+      report_fatal_error("Can't handle a single output filename and multiple "
----------------
There is a lot of code duplication between these various functions, consider refactoring possibly in a follow-on patch.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:414
@@ +413,3 @@
+
+      ThinGenerator.promote(*TheModule, *Index);
+      ThinGenerator.crossModuleImport(*TheModule, *Index);
----------------
Why is the import() step a superset of promote and import, whereas the other steps (e.g. optimize()) only doing one thing?

================
Comment at: tools/llvm-lto/llvm-lto.cpp:428
@@ +427,3 @@
+
+  void optimize() {
+    if (InputFilenames.size() != 1 && !OutputFilename.empty())
----------------
Untested.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:484
@@ -268,1 +483,3 @@
 
+  if (ThinLTOMode.getNumOccurrences()) {
+    thinlto::ThinLTOProcessing ThinLTOProcessor(Options);
----------------
What if there is more than one occurrence?


http://reviews.llvm.org/D17066