[PATCH] D54815: [ThinLTO] Add summary entries for index-based WPD

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 5 inline comments as done.
tejohnson added inline comments.


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:271
   FS_TYPE_ID = 21,
+  // Maps type identifier to summary information for that type identifier
+  // computed from type metadata: the valueid of each vtable definition
----------------
davidxl wrote:
> It is a little difficult to parse the comment here.  Is it just mapping from type id to global value id of the vtable?   Also what is 'vtable definition decorated with a type metadata?'
This is summarizing the type metadata described here:
https://llvm.org/docs/TypeMetadata.html

Each vtable def can be decorated with multiple type metadata that it is compatible with (due to inheritance). See the example below in test/Assembler/thinlto-vtable-summary.ll. E.g. B and C each derived from A so they each have 2 type metadata attachments. Therefore the typeIdMetadata summary for A includes both vtables.

I will try to add a more intuitive explanation, and point to the doc.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:818
+/// metadata.
+using TypeIdOffsetGVPair = std::pair<uint64_t, ValueInfo>;
+/// List of vtable definitions decorated by the same type id metadata,
----------------
davidxl wrote:
> Perhaps add that this is a mapping from type (id) to vtable info?
This one does not include the type id, it is the vtable def's ValueInfo and the byte offset from one of its attached type metadata (the offset is the offset into the vtable of the address point for that type as defined in the Itanium ABI: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vtable-general). The attached type metadata for a given vtable def are all the the types it is compatible with. It's a little hard to explain, the example in https://llvm.org/docs/TypeMetadata.html helps illustrate this. I can try to clarify and point to that doc.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:821
+/// and their corresponding offsets in the type id metadata.
+using TypeIdGVInfo = std::vector<TypeIdOffsetGVPair>;
+
----------------
davidxl wrote:
> Why is it a one-to-many mapping?
Because each type id may be compatible with multiple vtables, due to inheritance.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:841
+  /// analysis and consumed by thin link.
+  std::map<std::string, TypeIdGVInfo> TypeIdMetadataMap;
+
----------------
davidxl wrote:
> TypeIdMetadataMap -- the name sounds too generic.  Is it possible to rename it to carry more semantics?
Maybe TypeIdCompatibleVtableMap?


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:1214
+  /// (if not present). This may be used when importing.
+  const TypeIdGVInfo *getTypeIdMetadataSummary(StringRef TypeId) const {
+    auto I = TypeIdMetadataMap.find(TypeId);
----------------
davidxl wrote:
> Returns an ArrayRef?
Yeah I can change this.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54815/new/

https://reviews.llvm.org/D54815