[PATCH] D23599: [ThinLTO] Add caching to the new LTO API

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D23599#521562, @tejohnson wrote:

> I had a bit of trouble following the flow, so I added in some suggestions for more comments and some assertion checking.


I added a high level description above `class NativeObjectOutput` and `class CacheObjectOutput`. Let me know if it helps.


================
Comment at: include/llvm/LTO/Caching.h:49
@@ +48,3 @@
+
+  // Try loading from a possible cache first, return true on cache hit.
+  bool tryLoadFromCache(StringRef Key) override;
----------------
tejohnson wrote:
> "return true and set EntryPath on cache hit."
EntryPath is unconditionally set, I added this.

================
Comment at: lib/LTO/Caching.cpp:50
@@ +49,3 @@
+      OS << (*ReloadedBufferOrErr)->getBuffer();
+      AddBuffer(std::move(*ReloadedBufferOrErr));
+    }
----------------
tejohnson wrote:
> It seems wrong that we don't have a return after this, otherwise in the case where a new cache entry is written AddBuffer() will be called twice. I guess it is simply inefficient but will work - you will load again from EntryPath that was just written, and rewrite to the output file.
Right, indeed I didn't intend to callback here, we should delete the temporary and reload the cache entry (mmap) and call the callback.

================
Comment at: lib/LTO/LTO.cpp:88
@@ +87,3 @@
+
+  // Include the hash for the preserved symbols.
+  for (auto &GS : DefinedGlobals) {
----------------
tejohnson wrote:
> This is what the old version was doing, but here we don't have the list of preserved symbols here. Wouldn't this be the ExportedGUIDs set created in runThinLTO? That should presumably be passed down here too and used as in your existing cache key computation in ThinLTOCodeGenerator.
Since internalization is done on the summary now, we don't really need to hash the "preserved symbols". We hash the linkage type from the index instead.

I updated the comment to reflect the change in the code. 

================
Comment at: lib/LTO/LTO.cpp:92
@@ +91,3 @@
+    Hasher.update(
+        ArrayRef<uint8_t>((const uint8_t *)&Linkage, sizeof(Linkage)));
+  }
----------------
tejohnson wrote:
> This should be hashing GUIDs not linkage.
Let me know if the above comment is enough.

================
Comment at: lib/LTO/LTO.cpp:514
@@ +513,3 @@
+      assert(Task == TaskId && "Unexpexted TaskId mismatch");
+      return std::move(Output);
+    };
----------------
tejohnson wrote:
> What if caching not enabled? In that case we shouldn't call addOutput above, so we won't have an Output. It should presumably just call addOutput() here then before returning. Add a comment that this returns the cached output if enabled.
We need the output in the first place to be able to check if the caching is enabled or not.



https://reviews.llvm.org/D23599