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

[Mehdi AMINI via llvm-commits]

joker.eph marked 4 inline comments as done.

================
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.
----------------
tejohnson wrote:
> 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?
Mmmm, this is yet to be implemented.
I'd say it applies to what we store on disk when the linker is *not running*.  I.e. during the link you are allowed to use extra space without any limit.

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:47
@@ +46,3 @@
+/// Cache behavior controls.
+struct CachingOptions {
+  std::string Path;
----------------
tejohnson wrote:
> 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.
Now I'm unsure it was clear enough, because you wrote " the module may be removed earlier due to the pruning interval". A cache entry *can't* be remove earlier. The pruning interval means that will only *check* the entries.

================
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)
----------------
tejohnson wrote:
> Is this a TODO? If so, please add TODO comment.
Will do. 

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

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:330
@@ +329,3 @@
+
+  // Save temps: index.
+  if (!SaveTempsDir.empty()) {
----------------
tejohnson wrote:
> 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.
Good point.

(I expect this to come back with a very different summary-based implementation)

================
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");
----------------
tejohnson wrote:
> 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?
Good point, this is legacy from when there was all the internalization stage, will cleanup!



================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:378
@@ +377,2 @@
+    llvm::PrintStatistics();
+}
----------------
tejohnson wrote:
> 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.
OK.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:373
@@ +372,3 @@
+    for (auto &MemBuffer : InputBuffers)
+      ThinGenerator.addModule(MemBuffer->getBufferIdentifier(),
+                              MemBuffer->getBuffer());
----------------
tejohnson wrote:
> Adding all the modules isn't needed for promotion. Should be able to remove this loop.
It isn't needed... for now ;)
I wrote this code against my prototype of summary-based importing where the promotion is driven by what the other modules export.
(I'll remove for now).

================
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 "
----------------
tejohnson wrote:
> There is a lot of code duplication between these various functions, consider refactoring possibly in a follow-on patch.
OK

================
Comment at: tools/llvm-lto/llvm-lto.cpp:414
@@ +413,3 @@
+
+      ThinGenerator.promote(*TheModule, *Index);
+      ThinGenerator.crossModuleImport(*TheModule, *Index);
----------------
tejohnson wrote:
> Why is the import() step a superset of promote and import, whereas the other steps (e.g. optimize()) only doing one thing?
Same reason as above: when I wrote it against the pure summary-based importing, it was needed/helpful. I'll remove for now.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:428
@@ +427,3 @@
+
+  void optimize() {
+    if (InputFilenames.size() != 1 && !OutputFilename.empty())
----------------
tejohnson wrote:
> Untested.
Any suggestion on how to test that? This is basically like testing `opt -O3`?


================
Comment at: tools/llvm-lto/llvm-lto.cpp:484
@@ -268,1 +483,3 @@
 
+  if (ThinLTOMode.getNumOccurrences()) {
+    thinlto::ThinLTOProcessing ThinLTOProcessor(Options);
----------------
tejohnson wrote:
> What if there is more than one occurrence?
I'll add a check and error.


http://reviews.llvm.org/D17066