[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.
----------------
joker.eph wrote:
> 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.
I don't really understand what you mean by this, particularly the last sentence about using extra space during the link without any limit. Isn't this set/used during the link (which is running the ThinLTO steps in process)?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:47
@@ +46,3 @@
+/// Cache behavior controls.
+struct CachingOptions {
+  std::string Path;
----------------
joker.eph wrote:
> 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.
Ah, so a clarifying comment would help then since I misunderstood. It sounds like the modules will be checked for expiration (and pruned from the cache) at the pruning interval. So with the default pruning interval of -1, the expiration is unused? I took these to be separate, complimentary, mechanisms for keeping the cache size down. Another option beyond just documenting the interactions well is to combine these parameters into a single interface that takes both the pruning interval and the expiration. 

================
Comment at: tools/llvm-lto/llvm-lto.cpp:428
@@ +427,3 @@
+
+  void optimize() {
+    if (InputFilenames.size() != 1 && !OutputFilename.empty())
----------------
joker.eph wrote:
> tejohnson wrote:
> > Untested.
> Any suggestion on how to test that? This is basically like testing `opt -O3`?
> 
Not sure, maybe just invoke this stage in your test after the importing action to make sure it succeeds (without necessarily checking for any specific optimization)? 


http://reviews.llvm.org/D17066