[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 added a comment.

Looks good generally, but a few questions/comments.



================
Comment at: llvm/include/llvm-c/lto.h:651
+/**
+ * Returns a reference to the ith object file produced by the ThinLTO
+ * CodeGenerator.
----------------
Doc needs update - returns path to ...


================
Comment at: llvm/include/llvm-c/lto.h:658
+ *
+ * \since LTO_API_VERSION=18
+ */
----------------
Since version should be 21?


================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:87
    */
   std::vector<std::unique_ptr<MemoryBuffer>> &getProducedBinaries() {
     return ProducedBinaries;
----------------
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.


================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:97
+  std::vector<std::string> &getProducedBinaryFiles() {
+    return ProducedBinaryFiles;
+  }
----------------
Ditto


================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:294
+  /// Path to a directory to save the generated object files.
+  std::string SaveGeneratedObjects;
+
----------------
This variable name was confusing to me - can you change to something like SavedObjectsDirectoryPath or something like that (and change corresponding parameter names where this is passed in)?


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:740
+/// Write out the generated object file, either from CacheEntryPath or from
+/// OutputBuffer, preferring hard-link when possible.
+static std::string writeGeneratedObject(int count, StringRef CacheEntryPath,
----------------
Document return value


================
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))
----------------
What is the point of this line?


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:762
+    // output.
+    errs() << "error: can't link or copy from cached entry '" << CacheEntryPath
+           << "' to '" << OutputPath << "'\n";
----------------
I guess this is the case where the cache entry was somehow pruned between the time the OutputBuffer was loaded and when the copy/hardlink was attempted? Can you add a comment to that effect?


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:953
+          if (!CacheEntryPath.empty()) {
+            // Cache is enabled, reload from the cache
+            auto ReloadedBufferOrErr = CacheEntry.tryLoadingBuffer();
----------------
Remind we why we do this instead of just using the existing OutputBuffer?


================
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
----------------
Nit: why use ".o" extension for save objects which is a directory?


https://reviews.llvm.org/D27507