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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM, see small comments below. Error message I was suggesting is not a blocker, up to you if you agree.



================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:87
    */
   std::vector<std::unique_ptr<MemoryBuffer>> &getProducedBinaries() {
     return ProducedBinaries;
----------------
mehdi_amini wrote:
> 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?
Sure, it won't assert if invoked properly, I was suggesting a more meaningful error in case it is not.


================
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))
----------------
mehdi_amini wrote:
> 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)
Ok, thanks


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:953
+          if (!CacheEntryPath.empty()) {
+            // Cache is enabled, reload from the cache
+            auto ReloadedBufferOrErr = CacheEntry.tryLoadingBuffer();
----------------
mehdi_amini wrote:
> 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)
I see, because reloading will attempt to mmap the file. Nit: typo in new comment "pressuree"


https://reviews.llvm.org/D27507