[PATCH] D35189: Add GUID-ValueID map and original name to ThinLTO summary.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D35189#803332, @mehdi_amini wrote:

> The context and the motivation is *still* not clear to me. To begin why do we need the *names* when doing the thin link?


Right - I think we don't need the IRSymtab or strtab with this approach since the valueID->GUID correspondence is being recorded directly. We had talked about emitting the value IDs via the VST and the names via the symtab/strtab, but that doesn't work well (it's not that the VST has redundant info, as Haojie indicated in his comment, but rather that it doesn't have the right info - it no longer maps the value id to the name or even directly to a symtab entry). This issue just affects the summary description of this patch since the actual minimization is being left for a follow-on - Haojie, can you update the summary description of the patch?



================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5225
       if (!LastSeenSummary)
         return error("Name attachment that does not follow a combined record");
       LastSeenSummary->setOriginalName(OriginalName);
----------------
Error string needs update (not just a combined record, but any summary record now)


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:131
+  /// Map that holds the correspondence between GUIDs in the summary index
+  /// and a value id generated by this class to use in the VST and summary 
+  /// block records.
----------------
This isn't quite right - we are still only generating the value id in this class for guids from indirect call profiles. I would suggest going back to the old description and leaving the map as-is. More on that below.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:304
 
+void ModuleBitcodeWriter::setGUIDToValueIdMap() {
+  auto insertToGUIDToValueIdMap = [&](GlobalValue::GUID TheGUID, unsigned TheValueId) {
----------------
Needs a function description. Note that this needs to be called before we write out the summaries, since we write out the GUID->ValueID records first.

However, I don't think you need to do this at all. There is really no reason to fill up the map with the other GUID->valueId mappings since they can be obtained on the fly when we write the FS_VALUE_GUID records.

I'd suggest instead changing the code that writes the FS_VALUE_GUID records such that it not only walks the map (for the value Ids we have to synthesize here for indirect call profiles), but also does the below Module traversals to emit the rest of them (without ever adding them to the map).


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3436
   SmallVector<uint64_t, 64> NameVals;
+  // For local linkage, we also emit the original name separately
+  // immediately after the record.
----------------
the original name's GUID actually.


https://reviews.llvm.org/D35189