[PATCH] D34063: [ThinLTO] YAML traits for module summaries

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Can you add some test cases?



================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:247
+    io.mapOptional("Kind", summary.Kind);
+    io.mapOptional("Linkage", summary.Linkage);
+    io.mapOptional("NotEligibleToImport", summary.NotEligibleToImport);
----------------
ncharlie wrote:
> Some of these traits are duplicated. I could create a second struct called SharedSummaryYaml or something to deduplicate these fields, if anyone thinks they're worth centralizing. 
Yes I think that would be good. How about GlobalValueSummaryYaml, to mirror the class structure of the summaries themselves.


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:357
+          std::vector<CalleeInfoYaml> Calls;
+          for (auto edge : FSum->calls()) {
+            Calls.push_back(CalleeInfoYaml{
----------------
Also need to iterate over the refs() here and for alias and global var summaries (refs() is in the GlobalValueSummary base class)


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:406
+  static void inputOne(IO &io, StringRef Key, NameMapTy &V) {
+    // do nothing - this map is only used for output
+  }
----------------
Should there be an assert here that this is never invoked then?


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:420
+    if(M.Mod) {
+      for(Module::global_object_iterator i = M.Mod->global_object_begin(); i != M.Mod->global_object_end(); i++) {
+        NameMap.insert(std::make_pair(i->getGUID(), i->getName()));
----------------
Needs clang format. I believe you will also need to iterate over the aliases doing the same thing (global objects only includes functions and globalvars).


https://reviews.llvm.org/D34063