[PATCH] D33615: Move summary dead stripping before regular LTO and record results in the combined summary

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:132
+  unsigned Linkage;
+  bool NotEligibleToImport, Live;
   std::vector<uint64_t> TypeTests;
----------------
pcc wrote:
> mehdi_amini wrote:
> > The addition of Linkage and NotEligibleToImport are unrelated to this change right? I feel you could commit them separately now.
> > (even the serialization of "Live" here isn't clear to me if it isn't something that could just be split out).
> Would we be using Linkage and NotEligibleToImport for anything though? It seems a little odd to add fields here without any test coverage. (That's not to say that we shouldn't add the fields eventually, but I think it should be accompanied by tests, i.e. we should add a JSON summary dumper to llvm-lto2 or something and convert some tests over to use it.)
> 
> I think the serialization of Live was part of D33702, and I'd prefer it to be done as part of that change.
> Would we be using Linkage and NotEligibleToImport for anything though? It seems a little odd to add fields here without any test coverage.

What are we using the Yaml serialization for? I thought it was intended to be the "primary textual way of dumping summaries"? My comment was in this context: I was seeing this change as "fix missing serialization" and nothing else.

> (That's not to say that we shouldn't add the fields eventually, but I think it should be accompanied by tests, i.e. we should add a JSON summary dumper to llvm-lto2 or something and convert some tests over to use it.)

Of course I expect a test for these, but don't we have a test for yaml dump? I was just thinking about using `opt` to generate a bitcode file with summaries and pretty-print to yaml. (haven't looked at the existing testing right now)


Repository:
  rL LLVM

https://reviews.llvm.org/D33615