[PATCH] D28041: IR: Module summary representation for type identifiers; summary test scaffolding for lowertypetests.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:401
+  // FIXME: Add bitcode read/write support for this field.
+  std::map<std::string, TypeIdSummary> TypeIdMap;
+
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > Is this supposed to be "per module" or should it end up global to the index? (My understanding was the latter but you implemented the former AFAICT).
> > Mehdi: What makes this per-module vs global? The same ModuleSummaryIndex data structure is used for both the permodule index and the combined index.
> > 
> > pcc: Why no accessors for this map?
> Oh forget my comment, I mis-identified the enclosing class.
> pcc: Why no accessors for this map?

I don't need them yet. That will come in a later change.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:646
+
+} // End yaml namespace
 } // End llvm namespace
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > Could these all go in an implementation file with proper forward declaration here?
> Alternatively, what about extracting this into a separate header `llvm/include/llvm/IR/ModuleSummaryIndexSerialization.h` that would include `llvm/include/llvm/IR/ModuleSummaryIndex.h` (if necessary).
Separate header sounds better; done.


https://reviews.llvm.org/D28041