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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

I don't have any strong feeling against YAML, but we should take this decision for the full summary and not just for the "type test", so I'd like to hear from Teresa about that (in the past we were considering a LLVM "metadata" serialization instead, or similar, so that it can round-trip in the IR itself).

(I'm tired of the llvm-bcanalyzer tests for the summary, so any kind of textual serialization of the summaries for testing and debugging purpose will be welcome at this point)



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:401
+  // FIXME: Add bitcode read/write support for this field.
+  std::map<std::string, TypeIdSummary> TypeIdMap;
+
----------------
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).


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:646
+
+} // End yaml namespace
 } // End llvm namespace
----------------
Could these all go in an implementation file with proper forward declaration here?


================
Comment at: llvm/test/Transforms/LowerTypeTests/import-unsat.ll:23
+  ret i1 %x
+}
----------------
Can you add a one line comment about that this test is checking?


https://reviews.llvm.org/D28041