[PATCH] D27507: [ThinLTO] Add an API to trigger file-based API for returning objects to the linker

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks for the review! See inline.



================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:87
    */
   std::vector<std::unique_ptr<MemoryBuffer>> &getProducedBinaries() {
     return ProducedBinaries;
----------------
tejohnson wrote:
> Give an error here if the wrong routine called? I.e. if getProducedBinaries called and setGeneratedObjectsDirectory was called. I see that the C api will assert if the wrong interface is called because the index will be out of range, but perhaps a more meaningful error could be given.
There shouldn't be an assert in the C API if correctly used: 

The client is supposed to iterate over the range given by `thinlto_module_get_num_object_files()` and call `thinlto_module_get_object_file()` (similarly `thinlto_module_get_object`/`thinlto_module_get_num_objects`).

The way I implemented it in ld64 is basically:

```
for (int BufId = 0; BufID < thinlto_module_get_num_objects; ++BufID) {
   process(thinlto_module_get_object(BufID));
}
for (int FileID = 0; FileID < thinlto_module_get_num_object_files; ++FileID) {
   process(thinlto_module_get_object_file(FileID));
}
```

Does it make sense?


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:746
+  llvm::sys::path::append(OutputPath, Twine(count) + ".thinlto.o");
+  OutputPath.c_str();
+  if (sys::fs::exists(OutputPath))
----------------
tejohnson wrote:
> What is the point of this line?
This ensure that the SmallString is null terminated before calling `sys::fs::exists` (added a comment)


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:953
+          if (!CacheEntryPath.empty()) {
+            // Cache is enabled, reload from the cache
+            auto ReloadedBufferOrErr = CacheEntry.tryLoadingBuffer();
----------------
tejohnson wrote:
> Remind we why we do this instead of just using the existing OutputBuffer?
Lower memory consumption: the buffer is on the heap and releasing this memory can be used for the next file. The final binary link will reload from disk (or from the VFS cache if the memory pressure wasn't too high).
(Added comment to this effect)


================
Comment at: llvm/test/ThinLTO/X86/save_objects.ll:6
+; RUN: rm -Rf %t.thin.o
+; RUN: llvm-lto -thinlto-save-objects=%t.thin.o -thinlto-action=run %t2.bc  %t.bc -exported-symbol=main 
+; RUN: ls %t.thin.o | count 2
----------------
tejohnson wrote:
> Nit: why use ".o" extension for save objects which is a directory?
This is just what `Xcode` is doing, because originally it was implemented for Monolithic LTO where there is a single output file ;)
Changed it to `%t.thin.out`


https://reviews.llvm.org/D27507